This repository has been archived by the owner on Dec 8, 2021. It is now read-only.
Implement better session pooling. #307
Assignees
Labels
api: spanner
Issues related to the googleapis/google-cloud-cpp-spanner API.
type: feature request
‘Nice-to-have’ improvement, new feature or different behavior or design.
Milestone
Comments
coryan
added
the
type: feature request
|
Assigning to myself to do some research / design work on this; even if we don't implement it (fully or at all) for alpha, we've run into several case where other decisions are predicated on having some understanding of how we eventually expect it to work. |
|
ok, I've done some research on this, I'm going to unassign it until we're ready to work on it (since it's a Beta feature) - in the meantime #445 is a step in this direction. |
mr-salty
added a commit
that referenced
this issue
Oct 3, 2019
* Factor SessionPool out of ConnectionImpl. Make `SessionPool` its own class to avoid clutter in `ConnectionImpl` and to make it easier to test. Since `SessionPool` is a member of `ConnectionImpl`, which guarantees the former cannot outlive the latter; only `ConnectionImpl` is required to be reference counted. This simplifies lifetime management, avoiding some subtle issues and race conditions, at the cost of requiring some ostensibly pool-related private methods in `ConnectionImpl` for the benefit of `SessionHolder` There are no functional changes compared to the current implementation. Part of #307 * Apparently I failed to learn my lesson about make_unique. * address review comments * fix build and add some comments to SessionPool * fix the build for real this time and remove an extra {} * Argh - I knew those {} were there for a reason (else cxx17 complains)
mr-salty
added a commit
that referenced
this issue
Oct 8, 2019
* Use BatchCreateSessionsRequest to create sessions. Part of #307 * fix return type to work with all compilers. * fix windows by avoiding narrowing. * Fix signed/unsigned and narrowing issues. * Fix the new tests to use BatchCreateSessions Also fixed a typo in a comment/string
mr-salty
added a commit
that referenced
this issue
Nov 5, 2019
* feat: split `ConnectionImpl` and `SessionPool` cleanly Remove the circular dependency between `ConnectionImpl` and `SessionPool`. The latter can now make RPCs directly instead of having to call back through the `SessionManager` interface. The `SessionHolder` deleter also now calls directly into `SessionPool` so we don't need to forward those calls from `ConnectionImpl`. I also removed the tests where `BatchCreateSessions` returned `RESOURCE_EXHAUSTED` since we learned from spanner team that will never happen (since there are no longer server-side session limits), and the tests conflicted with our retry policy. Part of #307
mr-salty
added a commit
that referenced
this issue
Nov 11, 2019
* refactor: factor Session preparation to a helper This will facilitate some upcoming Session/Stub-related changes. I also added another helper to help build a result with a `StatusOnlyResultSetSource`. One subtler change is instead of doing `Session` preparation inside `ExecuteSqlImpl` we now do it in its two callers (`CommonQueryImpl` and `CommonDmlImpl`), that will also facilitate additional changes involving stubs (which are used in the body of the latter two methods). Part of #307
mr-salty
added a commit
that referenced
this issue
Nov 13, 2019
*`SessionPool` manages assigning a `SpannerStub` to each `Session` *`ConnectionImpl` obtains the stub to use for a call from the `SessionPool` and no longer retains any knowledge of the stub. This will enable the use of multiple stubs per `Connection`; it's important for a `Session` to remain associated with the stub/channel that created it, otherwise there is a performance penalty. Part of #307
mr-salty
added a commit
that referenced
this issue
Nov 19, 2019
* feat: support multiple channels in SessionPool * SessionPool now utilizes all channels that are given to it, creating new sessions on the least loaded channel. * The default number of channels created has been changed to 4. * Refactor Session creation and add to pool logic. * Add some tests that use multipe channels. * Rework some tests that depended on the order of returned Sessions. Part of #307 * address review comments * address review comments * address review comments * very important backtick change
mr-salty
added a commit
that referenced
this issue
Nov 21, 2019
* `max_sessions` is now `max_sessions_per_channel` * we're not going to be using `write_sessions_fraction`, so remove it. Part of #307
|
Any update on this? Looks like there's been lots of progress. Is this done? |
|
not done, I have one more PR in progress and then I think I can declare it "done enough to close this bug" and open issues for any small things leftover (in retrospect I probably should have opened multiple issues at the start instead of using this for everything) |
|
Can we file a separate issue that clearly describes what remaining feature/functionality we need to add in this next PR you're referring to? I'm thinking then we can probably label that issue with GA (not beta), then close this one for Beta. FTR, this is related to what Todd and I talked about offline. |
|
The 4 bullets I listed initially probably should have been separate issues. 3 of them are done: utilizing multiple channels, proactively creating I opened #1171 for |
google-cloud-label-sync
bot
added
the
api: spanner
devjgm
pushed a commit
to devjgm/google-cloud-cpp
that referenced
this issue
May 7, 2020
…p-spanner#916) * Factor SessionPool out of ConnectionImpl. Make `SessionPool` its own class to avoid clutter in `ConnectionImpl` and to make it easier to test. Since `SessionPool` is a member of `ConnectionImpl`, which guarantees the former cannot outlive the latter; only `ConnectionImpl` is required to be reference counted. This simplifies lifetime management, avoiding some subtle issues and race conditions, at the cost of requiring some ostensibly pool-related private methods in `ConnectionImpl` for the benefit of `SessionHolder` There are no functional changes compared to the current implementation. Part of googleapis/google-cloud-cpp-spanner#307 * Apparently I failed to learn my lesson about make_unique. * address review comments * fix build and add some comments to SessionPool * fix the build for real this time and remove an extra {} * Argh - I knew those {} were there for a reason (else cxx17 complains)
devjgm
pushed a commit
to devjgm/google-cloud-cpp
that referenced
this issue
May 7, 2020
…-cloud-cpp-spanner#924) * Use BatchCreateSessionsRequest to create sessions. Part of googleapis/google-cloud-cpp-spanner#307 * fix return type to work with all compilers. * fix windows by avoiding narrowing. * Fix signed/unsigned and narrowing issues. * Fix the new tests to use BatchCreateSessions Also fixed a typo in a comment/string
devjgm
pushed a commit
to devjgm/google-cloud-cpp
that referenced
this issue
May 7, 2020
…ogle-cloud-cpp-spanner#1022) * feat: split `ConnectionImpl` and `SessionPool` cleanly Remove the circular dependency between `ConnectionImpl` and `SessionPool`. The latter can now make RPCs directly instead of having to call back through the `SessionManager` interface. The `SessionHolder` deleter also now calls directly into `SessionPool` so we don't need to forward those calls from `ConnectionImpl`. I also removed the tests where `BatchCreateSessions` returned `RESOURCE_EXHAUSTED` since we learned from spanner team that will never happen (since there are no longer server-side session limits), and the tests conflicted with our retry policy. Part of googleapis/google-cloud-cpp-spanner#307
devjgm
pushed a commit
to devjgm/google-cloud-cpp
that referenced
this issue
May 7, 2020
…loud-cpp-spanner#1037) * refactor: factor Session preparation to a helper This will facilitate some upcoming Session/Stub-related changes. I also added another helper to help build a result with a `StatusOnlyResultSetSource`. One subtler change is instead of doing `Session` preparation inside `ExecuteSqlImpl` we now do it in its two callers (`CommonQueryImpl` and `CommonDmlImpl`), that will also facilitate additional changes involving stubs (which are used in the body of the latter two methods). Part of googleapis/google-cloud-cpp-spanner#307
devjgm
pushed a commit
to devjgm/google-cloud-cpp
that referenced
this issue
May 7, 2020
…ud-cpp-spanner#1041) *`SessionPool` manages assigning a `SpannerStub` to each `Session` *`ConnectionImpl` obtains the stub to use for a call from the `SessionPool` and no longer retains any knowledge of the stub. This will enable the use of multiple stubs per `Connection`; it's important for a `Session` to remain associated with the stub/channel that created it, otherwise there is a performance penalty. Part of googleapis/google-cloud-cpp-spanner#307
devjgm
pushed a commit
to devjgm/google-cloud-cpp
that referenced
this issue
May 7, 2020
…ud-cpp-spanner#1063) * feat: support multiple channels in SessionPool * SessionPool now utilizes all channels that are given to it, creating new sessions on the least loaded channel. * The default number of channels created has been changed to 4. * Refactor Session creation and add to pool logic. * Add some tests that use multipe channels. * Rework some tests that depended on the order of returned Sessions. Part of googleapis/google-cloud-cpp-spanner#307 * address review comments * address review comments * address review comments * very important backtick change
devjgm
pushed a commit
to devjgm/google-cloud-cpp
that referenced
this issue
May 7, 2020
…cloud-cpp-spanner#1089) * `max_sessions` is now `max_sessions_per_channel` * we're not going to be using `write_sessions_fraction`, so remove it. Part of googleapis/google-cloud-cpp-spanner#307
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
To unblock the implementation of
Client::Commit()I implemented a very naive session pool. Here are some of the things it does not do:CompletionQueueto do this with application-provided threads).The text was updated successfully, but these errors were encountered: