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: optimize maintainer to let sessions be GC'ed instead of deleted #135

Merged
merged 9 commits into from Apr 22, 2020

Conversation

olavloite
Copy link
Contributor

@olavloite olavloite commented Mar 30, 2020

Optimizes the session pool maintainer to:

  1. Let the backend garbage collect sessions instead of explicitly deleting them. This reduces the number of RPCs needed.
  2. Only keep MinSessions + MaxIdleSessions alive in the session pool. Let all other sessions automatically expire and remove these from the pool if they are not being used. This reduces the required number of RPCs to keep sessions alive.

@googlebot googlebot added the cla: yes label Mar 30, 2020
@olavloite olavloite added the api: spanner label Mar 30, 2020
@olavloite olavloite requested review from skuruppu and hengfengli Mar 30, 2020
@hengfengli
Copy link
Contributor

@hengfengli hengfengli commented Apr 1, 2020

@skuruppu This repo does not need a PR approval before merging. Do we need to change the config?

@hengfengli
Copy link
Contributor

@hengfengli hengfengli commented Apr 2, 2020

@olavloite (3) can be reverted because it turns out SELECT 1 is better.

@hengfengli
Copy link
Contributor

@hengfengli hengfengli commented Apr 2, 2020

I see why it doesn't need a PR approval because this is not going to merge into master. Only merging into master requires an approval.

@olavloite
Copy link
Contributor Author

@olavloite olavloite commented Apr 2, 2020

@olavloite (3) can be reverted because it turns out SELECT 1 is better.

Done

@olavloite
Copy link
Contributor Author

@olavloite olavloite commented Apr 2, 2020

I see why it doesn't need a PR approval because this is not going to merge into master. Only merging into master requires an approval.

I based this PR on the PR for batch increasing sessions in the pool, as that PR already includes the dependencies and setup for the benchmarking. It's best to merge that PR first into master, and then rebase this PR on master once that's been done.

@olavloite olavloite force-pushed the batch-increase-sessions branch from edfa509 to e3ac294 Compare Apr 4, 2020
Copy link
Contributor

@hengfengli hengfengli left a comment

LGTM. @skuruppu please take a quick look because this PR is quite large.

@olavloite olavloite changed the base branch from batch-increase-sessions to master Apr 8, 2020
@olavloite olavloite force-pushed the gc-sessions-instead-of-delete branch from 6401cb9 to c95aed6 Compare Apr 8, 2020
@olavloite olavloite added the kokoro:force-run label Apr 8, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run label Apr 8, 2020
sessionToKeepAlive = findSessionToKeepAlive(readSessions, keepAliveThreshold, 0);
if (sessionToKeepAlive == null) {
sessionToKeepAlive = findSessionToKeepAlive(writePreparedSessions, keepAliveThreshold);
sessionToKeepAlive =
findSessionToKeepAlive(
writePreparedSessions, keepAliveThreshold, readSessions.size());
Copy link
Contributor

@skuruppu skuruppu Apr 15, 2020

Choose a reason for hiding this comment

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

Doesn't this logic meant that over time we won't be maintaining the ratio of write prepared sessions to read sessions since we always favor keeping read sessions alive?

Copy link
Contributor Author

@olavloite olavloite Apr 16, 2020

Choose a reason for hiding this comment

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

Yes, the session that is kept alive is taken from the pool, pinged and then released back into the pool (on line 1074). Releasing the session back into the pool will trigger a new prepareSession call if necessary.

I've added an additional test case to verify this behavior.

Copy link
Contributor Author

@olavloite olavloite Apr 16, 2020

Choose a reason for hiding this comment

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

And that test case now shows that my line of thought does not always work as expected :-)

@olavloite olavloite added the kokoro:force-run label Apr 16, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run label Apr 16, 2020
olavloite added 9 commits Apr 21, 2020
When more sessions are requested by the user application than are available in the session pool,
the session pool will now create new sessions in batches instead of in steps of 1. This reduces
the number of RPCs needed to serve a burst of requests.

A benchmark for the session pool has also been added to be able to compare performance and the
number of RPCs needed before and after this change. This benchmark can also be used for future
changes to verify that the change does not deteriorate performance or increase the number of
RPCs needed.
@olavloite olavloite force-pushed the gc-sessions-instead-of-delete branch from 6ede807 to a0dfa08 Compare Apr 21, 2020
@olavloite olavloite added the kokoro:force-run label Apr 21, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run label Apr 21, 2020
@olavloite olavloite added the kokoro:force-run label Apr 21, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run label Apr 21, 2020
@olavloite olavloite added the kokoro:force-run label Apr 21, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run label Apr 21, 2020
@olavloite olavloite added the kokoro:force-run label Apr 21, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run label Apr 21, 2020
@skuruppu
Copy link
Contributor

@skuruppu skuruppu commented Apr 21, 2020

Thanks for the changes @olavloite. Please merge if you think it's ready to go. After that, I'll try to cut a release since we haven't done one in a while.

@skuruppu skuruppu merged commit d65747c into master Apr 22, 2020
12 of 13 checks passed
@skuruppu skuruppu deleted the gc-sessions-instead-of-delete branch Apr 22, 2020
gcf-merge-on-green bot pushed a commit that referenced this issue Apr 22, 2020
🤖 I have created a release \*beep\* \*boop\* 
---
## [1.53.0](https://www.github.com/googleapis/java-spanner/compare/v1.52.0...v1.53.0) (2020-04-22)


### Features

* optimize maintainer to let sessions be GC'ed instead of deleted ([#135](https://www.github.com/googleapis/java-spanner/issues/135)) ([d65747c](https://www.github.com/googleapis/java-spanner/commit/d65747cbc704508f6f1bcef6eea53aa411d42ee2))


### Bug Fixes

* assign unique id's per test case ([#129](https://www.github.com/googleapis/java-spanner/issues/129)) ([a553b6d](https://www.github.com/googleapis/java-spanner/commit/a553b6d48c4f5ee2d0583e5b825d73a85f06216e))
* check for not null input for Id classes ([#159](https://www.github.com/googleapis/java-spanner/issues/159)) ([ecf5826](https://www.github.com/googleapis/java-spanner/commit/ecf582670818f32e85f534ec400d0b8d31cf9ca6)), closes [#145](https://www.github.com/googleapis/java-spanner/issues/145)
* clean up test instance if creation failed ([#162](https://www.github.com/googleapis/java-spanner/issues/162)) ([ff571e1](https://www.github.com/googleapis/java-spanner/commit/ff571e16a45fbce692d9bb172749ff15fafe7a9c))
* fix flaky test and remove warnings ([#153](https://www.github.com/googleapis/java-spanner/issues/153)) ([d534e35](https://www.github.com/googleapis/java-spanner/commit/d534e350346b0c9ab8057ede36bc3aac473c0b06)), closes [#146](https://www.github.com/googleapis/java-spanner/issues/146)
* increase test timeout and remove warnings ([#160](https://www.github.com/googleapis/java-spanner/issues/160)) ([63a6bd8](https://www.github.com/googleapis/java-spanner/commit/63a6bd8be08a56d002f58bc2cdb2856ad0dc5fa3)), closes [#158](https://www.github.com/googleapis/java-spanner/issues/158)
* retry non-idempotent long-running RPCs ([#141](https://www.github.com/googleapis/java-spanner/issues/141)) ([4669c02](https://www.github.com/googleapis/java-spanner/commit/4669c02a24e0f7b1d53c9edf5ab7b146b4116960))
* retry restore if blocked by pending restore ([#119](https://www.github.com/googleapis/java-spanner/issues/119)) ([220653d](https://www.github.com/googleapis/java-spanner/commit/220653d8e25c518d0df447bf777a7fcbf04a01ca)), closes [#118](https://www.github.com/googleapis/java-spanner/issues/118)
* StatementParser did not accept multiple query hints ([#170](https://www.github.com/googleapis/java-spanner/issues/170)) ([ef41a6e](https://www.github.com/googleapis/java-spanner/commit/ef41a6e503f218c00c16914aa9c1433d9b26db13)), closes [#163](https://www.github.com/googleapis/java-spanner/issues/163)
* wait for initialization to finish before test ([#161](https://www.github.com/googleapis/java-spanner/issues/161)) ([fe434ff](https://www.github.com/googleapis/java-spanner/commit/fe434ff7068b4b618e70379c224e1c5ab88f6ba1)), closes [#146](https://www.github.com/googleapis/java-spanner/issues/146)


### Performance Improvements

* increase sessions in the pool in batches ([#134](https://www.github.com/googleapis/java-spanner/issues/134)) ([9e5a1cd](https://www.github.com/googleapis/java-spanner/commit/9e5a1cdaacf71147b67681861f063c3276705f44))
* prepare sessions with r/w tx in-process ([#152](https://www.github.com/googleapis/java-spanner/issues/152)) ([2db27ce](https://www.github.com/googleapis/java-spanner/commit/2db27ce048efafaa3c28b097de33518747011465)), closes [#151](https://www.github.com/googleapis/java-spanner/issues/151)


### Dependencies

* update core dependencies ([#109](https://www.github.com/googleapis/java-spanner/issues/109)) ([5753f1f](https://www.github.com/googleapis/java-spanner/commit/5753f1f4fed83df87262404f7a7ba7eedcd366cb))
* update core dependencies ([#132](https://www.github.com/googleapis/java-spanner/issues/132)) ([77c1558](https://www.github.com/googleapis/java-spanner/commit/77c1558652ee00e529674ac3a2dcf3210ef049fa))
* update dependency com.google.api:api-common to v1.9.0 ([#127](https://www.github.com/googleapis/java-spanner/issues/127)) ([b2c744f](https://www.github.com/googleapis/java-spanner/commit/b2c744f01a4d5a8981df5ff900f3536c83265a61))
* update dependency com.google.guava:guava-bom to v29 ([#147](https://www.github.com/googleapis/java-spanner/issues/147)) ([3fe3ae0](https://www.github.com/googleapis/java-spanner/commit/3fe3ae02376af552564c93c766f562d6454b7ac1))
* update dependency io.grpc:grpc-bom to v1.29.0 ([#164](https://www.github.com/googleapis/java-spanner/issues/164)) ([2d2ce5c](https://www.github.com/googleapis/java-spanner/commit/2d2ce5ce4dc8f410ec671e542e144d47f39ab40b))
* update dependency org.threeten:threetenbp to v1.4.3 ([#120](https://www.github.com/googleapis/java-spanner/issues/120)) ([49d1abc](https://www.github.com/googleapis/java-spanner/commit/49d1abcb6c9c48762dcf0fe1466ab107bf67146b))
---


This PR was generated with [Release Please](https://github.com/googleapis/release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants