perf: close sessions async #24

olavloite · 2020-01-09T13:56:10Z

Sessions should be closed using an async gRPC call in order to speed up the closing of a large session pool. Instead of using its own executor, which is limited to 8 worker threads, to execute asynchronous delete session calls, the session pool should use the asynchronous call option in gRPC. This allows a larger number of asynchronous delete session calls to be executed in parallel and speeds up closing a session pool with a large number of sessions.

Testing against a real Cloud Spanner database with a session pool containing 1,000 sessions shows the following performance for closing the session pool:

Without this change (3 test runs):
6603ms
8169ms
8367ms

With this change (3 test runs):
1297ms
1710ms
1851ms

Fixes #19

olavloite · 2020-01-09T14:10:48Z

The binary compatibility check fails because of:

[ERROR] 7012: com.google.cloud.spanner.Session: Method 'public com.google.api.core.ApiFuture asyncClose()' has been added to an interface
[ERROR] 7012: com.google.cloud.spanner.spi.v1.SpannerRpc: Method 'public com.google.api.core.ApiFuture asyncDeleteSession(java.lang.String, java.util.Map)' has been added to an interface

Both Session and SpannerRpc are public interfaces, but are only used internally in the client library. There are no public methods returning either a Session or a SpannerRpc.

Sessions should be closed using an async gRPC call in order to speed up the closing of a large session pool. Instead of using its own executor, which is limited to 8 worker threads, to execute asynchronous delete session calls, the session pool should use the asynchronous call option in gRPC. This allows a larger number of asynchronous delete session calls to be executed in parallel and speeds up closing a session pool with a large number of sessions. Testing against a real Cloud Spanner database with a session pool containing 1,000 sessions shows the following performance for closing the session pool: Before (3 runs): 6603ms 8169ms 8367ms After (3 runs): 1297ms 1710ms 1851ms Fixes #19.

google-cloud-spanner/src/test/java/com/google/cloud/spanner/SessionPoolStressTest.java

hengfengli

LGTM, thanks for the benchmarking test 👍.

olavloite · 2020-01-19T17:43:59Z

@skuruppu @hengfengli @chingor13
This PR cannot be merged because the Binary Compatibility test fails. This happens because it adds a method to two public interfaces. These interfaces are not really public (as in: returned to users), but must be public because they reside in different packages within the Spanner client library and are used by different parts of the Spanner client lib.

So:

Should we still treat this as a breaking change?
Regardless of whether we do treat is a breaking change or not: What action is needed to make the PR mergeable?

skuruppu · 2020-01-21T03:50:01Z

@skuruppu @hengfengli @chingor13
This PR cannot be merged because the Binary Compatibility test fails. This happens because it adds a method to two public interfaces. These interfaces are not really public (as in: returned to users), but must be public because they reside in different packages within the Spanner client library and are used by different parts of the Spanner client lib.

So:

Should we still treat this as a breaking change?

I don't believe it should be flagged as a breaking change given that we should reserve those when making backwards-incompatible changes that requires users to rebuild their apps.

Regardless of whether we do treat is a breaking change or not: What action is needed to make the PR mergeable?

I'm not sure actually :( @kolea2 would you be able to advice us on whether it's ok to ignore a Binary Compatibility failure and merge this change.

I don't know why the Java 11 test fails so I triggered a rebuild.

skuruppu · 2020-01-23T02:51:29Z

@skuruppu @hengfengli @chingor13
This PR cannot be merged because the Binary Compatibility test fails. This happens because it adds a method to two public interfaces. These interfaces are not really public (as in: returned to users), but must be public because they reside in different packages within the Spanner client library and are used by different parts of the Spanner client lib.
So:

Should we still treat this as a breaking change?

I don't believe it should be flagged as a breaking change given that we should reserve those when making backwards-incompatible changes that requires users to rebuild their apps.

Regardless of whether we do treat is a breaking change or not: What action is needed to make the PR mergeable?

I'm not sure actually :( @kolea2 would you be able to advice us on whether it's ok to ignore a Binary Compatibility failure and merge this change.

I don't know why the Java 11 test fails so I triggered a rebuild.

As discussed offline, it's ok to push this change despite the Binary Compatibility failure since the changes being made are to internal interfaces.

skuruppu · 2020-01-23T04:04:17Z

@skuruppu @hengfengli @chingor13
This PR cannot be merged because the Binary Compatibility test fails. This happens because it adds a method to two public interfaces. These interfaces are not really public (as in: returned to users), but must be public because they reside in different packages within the Spanner client library and are used by different parts of the Spanner client lib.
So:

Should we still treat this as a breaking change?

I don't believe it should be flagged as a breaking change given that we should reserve those when making backwards-incompatible changes that requires users to rebuild their apps.

Regardless of whether we do treat is a breaking change or not: What action is needed to make the PR mergeable?

I'm not sure actually :( @kolea2 would you be able to advice us on whether it's ok to ignore a Binary Compatibility failure and merge this change.
I don't know why the Java 11 test fails so I triggered a rebuild.

As discussed offline, it's ok to push this change despite the Binary Compatibility failure since the changes being made are to internal interfaces.

Yikes, it turns out the compatibility check just keeps failing for all future PRs as well. I was hoping that it would stop complaining after it was merged in. Let me check with @kolea2 offline about what we can do to fix this issue.

hengfengli · 2020-01-23T05:04:42Z

Should we revert this change? We can wait until it is resolved so that we can merge it in.

skuruppu · 2020-01-23T05:05:26Z

Should we revert this change? We can wait until it is resolved so that we can merge it in.

Yeah good point. I'll revert it now.

This reverts commit ab25087.

This reverts commit 809ed88.

asyncClose() was added to com.google.cloud.spanner.Session and asyncDeleteSession() was added to com.google.cloud.spanner.spi.v1.SpannerRpc in #24 which resulted in binary compatibility test failures. This config allows us to ignore the failure.

@internalapi

* Revert "Revert "perf: close sessions async (#24)" (#43)" This reverts commit 809ed88. * Ignore compatibility check failure in internal interfaces. asyncClose() was added to com.google.cloud.spanner.Session and asyncDeleteSession() was added to com.google.cloud.spanner.spi.v1.SpannerRpc in #24 which resulted in binary compatibility test failures. This config allows us to ignore the failure. * Annotate SpannerRpc and Session classes as @internalapi. Users shouldn't be implementing these interfaces as they're internal to the client library implementation.

Fixes googleapis#13

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jan 9, 2020

olavloite added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 9, 2020

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 9, 2020

olavloite added 5 commits January 14, 2020 16:24

fix: wait for test servers to terminate

5b1ee9c

remove tracing for async call

426ab82

do not use directExecutor which could be a gRPC thread

d2ed3bc

fix: return failed future instead of throwing exception

a2be792

olavloite force-pushed the close-sessions-async branch from b3cfae4 to a2be792 Compare January 14, 2020 17:34

olavloite added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 14, 2020

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 14, 2020

olavloite requested review from skuruppu and hengfengli January 15, 2020 10:42

skuruppu approved these changes Jan 16, 2020

View reviewed changes

google-cloud-spanner/src/test/java/com/google/cloud/spanner/SessionPoolStressTest.java Outdated Show resolved Hide resolved

fix: remove commented code

8429fc2

hengfengli approved these changes Jan 17, 2020

View reviewed changes

olavloite added semver: major Hint for users that this is an API breaking change. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Jan 18, 2020

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 18, 2020

olavloite changed the title ~~perf: close sessions async~~ perf!: close sessions async Jan 18, 2020

olavloite added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 18, 2020

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 18, 2020

olavloite changed the title ~~perf!: close sessions async~~ perf: close sessions async Jan 18, 2020

olavloite removed the semver: major Hint for users that this is an API breaking change. label Jan 18, 2020

skuruppu merged commit ab25087 into master Jan 23, 2020

skuruppu deleted the close-sessions-async branch January 23, 2020 02:51

skuruppu added a commit that referenced this pull request Jan 23, 2020

Revert "perf: close sessions async (#24)"

c50bbb5

This reverts commit ab25087.

skuruppu mentioned this pull request Jan 23, 2020

Revert "perf: close sessions async" #43

Merged

hengfengli pushed a commit that referenced this pull request Jan 23, 2020

Revert "perf: close sessions async (#24)" (#43)

809ed88

This reverts commit ab25087.

hengfengli restored the close-sessions-async branch January 23, 2020 05:37

release-please bot mentioned this pull request Jan 23, 2020

chore: release 1.49.1 #42

Merged

yoshi-automation mentioned this pull request Jan 23, 2020

chore(regen): regenerate with updated license year #44

Merged

skuruppu added a commit that referenced this pull request Jan 24, 2020

Revert "Revert "perf: close sessions async (#24)" (#43)"

41f47db

This reverts commit 809ed88.

skuruppu mentioned this pull request Jan 24, 2020

perf: close sessions async revert revert #46

Merged

rajatbhatta pushed a commit to rajatbhatta/java-spanner that referenced this pull request Nov 17, 2022

chore: fix distribution_name in .repo-metadata (googleapis#24)

03148c4

Fixes googleapis#13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf: close sessions async #24

perf: close sessions async #24

olavloite commented Jan 9, 2020 •

edited

olavloite commented Jan 9, 2020

hengfengli left a comment

olavloite commented Jan 19, 2020

skuruppu commented Jan 21, 2020

skuruppu commented Jan 23, 2020

skuruppu commented Jan 23, 2020

hengfengli commented Jan 23, 2020

skuruppu commented Jan 23, 2020

perf: close sessions async #24

perf: close sessions async #24

Conversation

olavloite commented Jan 9, 2020 • edited

olavloite commented Jan 9, 2020

hengfengli left a comment

Choose a reason for hiding this comment

olavloite commented Jan 19, 2020

skuruppu commented Jan 21, 2020

skuruppu commented Jan 23, 2020

skuruppu commented Jan 23, 2020

hengfengli commented Jan 23, 2020

skuruppu commented Jan 23, 2020

olavloite commented Jan 9, 2020 •

edited