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

perf: close sessions async #24

Merged
merged 6 commits into from Jan 23, 2020
Merged

perf: close sessions async #24

merged 6 commits into from Jan 23, 2020

Conversation

olavloite
Copy link
Contributor

@olavloite olavloite commented Jan 9, 2020

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

@googlebot googlebot added the cla: yes label Jan 9, 2020
@olavloite
Copy link
Contributor Author

@olavloite olavloite commented Jan 9, 2020

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.

@olavloite olavloite added the kokoro:force-run label Jan 9, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run label Jan 9, 2020
olavloite added 5 commits Jan 14, 2020
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.
@olavloite olavloite added the kokoro:force-run label Jan 14, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run label Jan 14, 2020
@olavloite olavloite requested review from skuruppu and hengfengli Jan 15, 2020
Copy link
Contributor

@hengfengli hengfengli left a comment

LGTM, thanks for the benchmarking test 👍.

@olavloite olavloite added semver: major kokoro:force-run labels Jan 18, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run label Jan 18, 2020
@olavloite olavloite changed the title perf: close sessions async perf!: close sessions async Jan 18, 2020
@olavloite olavloite added the kokoro:force-run label Jan 18, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run label Jan 18, 2020
@olavloite olavloite changed the title perf!: close sessions async perf: close sessions async Jan 18, 2020
@olavloite olavloite removed the semver: major label Jan 18, 2020
@olavloite
Copy link
Contributor Author

@olavloite olavloite commented Jan 19, 2020

@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:

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

@skuruppu
Copy link
Contributor

@skuruppu skuruppu commented Jan 21, 2020

@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:

  1. 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.

  1. 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
Copy link
Contributor

@skuruppu skuruppu commented Jan 23, 2020

@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:

  1. 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.

  1. 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 skuruppu merged commit ab25087 into master Jan 23, 2020
10 of 11 checks passed
@skuruppu skuruppu deleted the close-sessions-async branch Jan 23, 2020
@skuruppu
Copy link
Contributor

@skuruppu skuruppu commented Jan 23, 2020

@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:

  1. 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.

  1. 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
Copy link
Contributor

@hengfengli hengfengli commented Jan 23, 2020

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

@skuruppu
Copy link
Contributor

@skuruppu skuruppu commented Jan 23, 2020

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.

skuruppu added a commit that referenced this issue Jan 23, 2020
hengfengli pushed a commit that referenced this issue Jan 23, 2020
@hengfengli hengfengli restored the close-sessions-async branch Jan 23, 2020
@release-please release-please bot mentioned this pull request Jan 23, 2020
skuruppu added a commit that referenced this issue Jan 24, 2020
skuruppu added a commit that referenced this issue Jan 24, 2020
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.
olavloite pushed a commit that referenced this issue Jan 24, 2020
* 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants