Skip to content

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

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

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 1 commit
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
Prev Previous commit
Merge branch 'main' into async-executor-threads-property
  • Loading branch information
@olavloite
olavloite committed Oct 4, 2023
commit 3ab5f0543a69dbd0ffa0d5f2ec4dc5599570e871
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
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.23.0')
implementation platform('com.google.cloud:libraries-bom:26.24.0')

implementation 'com.google.cloud:google-cloud-spanner'
```
Expand Down
15 changes: 14 additions & 1 deletion 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 @@ -918,6 +918,10 @@ public void testCustomAsyncExecutorProvider() {
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(
Expand All @@ -940,12 +944,21 @@ public void testAsyncExecutorProviderCoreThreadCount() throws Exception {
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);
System.setProperty(propertyName, propertyValue);
if (propertyValue == null) {
System.clearProperty(propertyName);
} else {
System.setProperty(propertyName, propertyValue);
}
try {
return callable.call();
} finally {
Expand Down
You are viewing a condensed version of this merge commit. You can view the full changes here.