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.

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
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ If you are using Maven without the BOM, add this to your dependencies:
If you are using Gradle 5.x or later, add this to your dependencies:

```Groovy
implementation platform('com.google.cloud:libraries-bom:26.22.0')
implementation platform('com.google.cloud:libraries-bom:26.23.0')

implementation 'com.google.cloud:google-cloud-spanner'
```
Expand Down
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";

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
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,49 @@ 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

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(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));
}

static <V> V runWithSystemProperty(
String propertyName, String propertyValue, Callable<V> callable) throws Exception {
String currentValue = System.getProperty(propertyName);
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