Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support setting core pool size for async API in system property #2632

Merged
merged 3 commits into from
Oct 4, 2023
Merged

feat: support setting core pool size for async API in system property #2632

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a39d9b3
feat: support setting core pool size for async API in system property
olavloite Sep 18, 2023
1006d0a
🦉 Updates from OwlBot post-processor
gcf-owl-bot[bot] Sep 18, 2023
3ab5f05
Merge branch 'main' into async-executor-threads-property
olavloite Oct 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 24 additions & 1 deletion google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerOptions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -524,7 +524,30 @@ public static FixedCloseableExecutorProvider create(ScheduledExecutorService exe
*/
@VisibleForTesting
static CloseableExecutorProvider createDefaultAsyncExecutorProvider() {
return createAsyncExecutorProvider(8, 60L, TimeUnit.SECONDS);
return createAsyncExecutorProvider(
getDefaultAsyncExecutorProviderCoreThreadCount(), 60L, TimeUnit.SECONDS);
}

@VisibleForTesting
static int getDefaultAsyncExecutorProviderCoreThreadCount() {
String propertyName = "SPANNER_ASYNC_NUM_CORE_THREADS";
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

magic string!

should we maintain list of constants/enums property names?

String propertyValue = System.getProperty(propertyName, "8");
try {
int corePoolSize = Integer.parseInt(propertyValue);
if (corePoolSize < 0) {
throw SpannerExceptionFactory.newSpannerException(
ErrorCode.INVALID_ARGUMENT,
String.format(
"The value for %s must be >=0. Invalid value: %s", propertyName, propertyValue));
}
return corePoolSize;
} catch (NumberFormatException exception) {
throw SpannerExceptionFactory.newSpannerException(
ErrorCode.INVALID_ARGUMENT,
String.format(
"The %s system property must be a valid integer. The value %s could not be parsed as an integer.",
propertyName, propertyValue));
}
}

/**
Expand Down
57 changes: 57 additions & 0 deletions google-cloud-spanner/src/test/java/com/google/cloud/spanner/SpannerOptionsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.concurrent.Callable;
import java.util.concurrent.ScheduledExecutorService;
import javax.annotation.Nonnull;
import org.junit.Test;
Expand Down Expand Up @@ -913,6 +914,62 @@ public void testCustomAsyncExecutorProvider() {
assertSame(service, options.getAsyncExecutorProvider().getExecutor());
}

@Test
public void testAsyncExecutorProviderCoreThreadCount() throws Exception {
assertEquals(8, SpannerOptions.getDefaultAsyncExecutorProviderCoreThreadCount());
Copy link
Contributor

@arpan14 arpan14 Oct 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Should we add a check to ensure that the system property was originally unset? Or maybe explicitly set the property to null?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that there is a handling of this scenario within the finally block of runWithSystemProperty method, but thought if explicitly setting null would be better to ensure that the default behaviour is due to null property and not because somehow the property was already set to 8.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a test for both null and an empty string.

String propertyName = "SPANNER_ASYNC_NUM_CORE_THREADS";
assertEquals(
Integer.valueOf(8),
runWithSystemProperty(
propertyName, null, SpannerOptions::getDefaultAsyncExecutorProviderCoreThreadCount));
assertEquals(
Integer.valueOf(16),
runWithSystemProperty(
propertyName, "16", SpannerOptions::getDefaultAsyncExecutorProviderCoreThreadCount));
assertEquals(
Integer.valueOf(1),
runWithSystemProperty(
propertyName, "1", SpannerOptions::getDefaultAsyncExecutorProviderCoreThreadCount));
assertThrows(
SpannerException.class,
() ->
runWithSystemProperty(
propertyName,
"foo",
SpannerOptions::getDefaultAsyncExecutorProviderCoreThreadCount));
assertThrows(
SpannerException.class,
() ->
runWithSystemProperty(
propertyName,
"-1",
SpannerOptions::getDefaultAsyncExecutorProviderCoreThreadCount));
assertThrows(
SpannerException.class,
() ->
runWithSystemProperty(
propertyName, "", SpannerOptions::getDefaultAsyncExecutorProviderCoreThreadCount));
}

static <V> V runWithSystemProperty(
String propertyName, String propertyValue, Callable<V> callable) throws Exception {
String currentValue = System.getProperty(propertyName);
if (propertyValue == null) {
System.clearProperty(propertyName);
} else {
System.setProperty(propertyName, propertyValue);
}
try {
return callable.call();
} finally {
if (currentValue == null) {
System.clearProperty(propertyName);
} else {
System.setProperty(propertyName, currentValue);
}
}
}

@Test
public void testDefaultNumChannelsWithGrpcGcpExtensionEnabled() {
SpannerOptions options =
Expand Down
Loading