feat: associate SpannerStub with a Session
#1041
Conversation
googlebot
added
the
cla: yes
|
google/cloud/spanner/internal/connection_impl.cc, line 518 at r1 (raw file):
FYI, I suspect the Is there any way to accomplish the goal of not doing an extra ref/unref other than having Does just doing |
Codecov Report
@@ Coverage Diff @@
## master #1041 +/- ##
==========================================
- Coverage 94.82% 94.78% -0.04%
==========================================
Files 158 160 +2
Lines 10610 10628 +18
==========================================
+ Hits 10061 10074 +13
- Misses 549 554 +5
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Reviewed 6 of 8 files at r1.
Reviewable status: 6 of 8 files reviewed, 6 unresolved discussions (waiting on @coryan, @devbww, @devjgm, @mr-salty, and @scotthart)
google/cloud/spanner/internal/connection_impl.cc, line 296 at r1 (raw file):
* an error if `session` is empty and no `Session` can be allocated. */ Status ConnectionImpl::PrepareSession(SessionHolder& session,
Consider moving this function to SessionPool, calling it PrepareSessionForCall() and have it return a StatusOr<std::pair<Session, Stub>>. I think the association between sessions and stubs should be a thing that only the session pool knows about.
google/cloud/spanner/internal/connection_impl.cc, line 518 at r1 (raw file):
Does just doing session->stub()->Call() now bump the reference count as well or can that be optimized away?
I would say it does incr/decr the refcount. There is nothing, for example, that might stop a concurrent "session->clear_stub()" call from happening between the return of stub() and the call of Call().
That is true, but safe. The dangerous thing is a call to session->clear_stub() before a call to session->stub()->Call() that is a crash.
google/cloud/spanner/internal/session_pool.cc, line 145 at r1 (raw file):
Previously, devbww (Bradley White) wrote…
Does it make any additional sense to move this logic into
Session. That is,session->AssignStubIfNeeded(stub_)?
+1 to that.
`SessionPool` manages assigning a `SpannerStub` to each `Session` `ConnectionImpl` obtains the stub to use for a call from the `Session` 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
There was a problem hiding this comment.
Reviewable status: 6 of 8 files reviewed, 6 unresolved discussions (waiting on @coryan, @devbww, @devjgm, and @scotthart)
google/cloud/spanner/transaction.cc, line 100 at r1 (raw file):
Previously, devbww (Bradley White) wrote…
At the moment it doesn't look like MakeDissociatedSessionHolder() really needs to be a template. Might it just take a
std::string session_nameand then be able to provide thenullptrto theSessionctor itself?
Done.
google/cloud/spanner/internal/connection_impl.cc, line 296 at r1 (raw file):
Previously, coryan (Carlos O'Ryan) wrote…
Consider moving this function to
SessionPool, calling itPrepareSessionForCall()and have it return aStatusOr<std::pair<Session, Stub>>. I think the association between sessions and stubs should be a thing that only the session pool knows about.
I like this idea but I think it ends up being a little weird because of the in/out nature of the SessionHolder and it not being copyable.
google/cloud/spanner/internal/connection_impl.cc, line 518 at r1 (raw file):
Previously, coryan (Carlos O'Ryan) wrote…
Does just doing session->stub()->Call() now bump the reference count as well or can that be optimized away?
I would say it does incr/decr the refcount. There is nothing, for example, that might stop a concurrent "session->clear_stub()" call from happening between the return of stub() and the call of Call().
That is true, but safe. The dangerous thing is a call to
session->clear_stub()before a call tosession->stub()->Call()that is a crash.
I think Brad was saying that if I changed stub() to return a reference to the shared_ptr instead of returning it by value (which seems to be the only way to prevent this extra refcount bump/decrement - is that something we really care about avoiding?), then a clear_stub() call (or another set_stub()) would be a problem.
In reality returning a reference would still be "safe" because there is no clear_stub() and set_stub() will only be called once, before any call to stub(). Although, I'd argue that returning a reference to a smart pointer is inherently an unsafe API. I really didn't want to have set_stub() at all, then it's clear the stub is set in the constructor and never changes. bww and I tossed around some ideas about that yesterday, but playing around with them they seemed strictly worse, requiring adding extra methods on Connection or extra members to *Params structs. The way it's done in this PR effectively keeps all of that contained in internal namespace things, at the expense of awkward (internal) APIs like this.
I still feel like there is probably a better solution, but also felt like I spun my wheels on this long enough. Carlos' suggestion above about returning the stub from a SessionPool method would be nice because I could then make the stub-related methods on Session private with SessionPool as a friend. But, it does always bump the reference count.
I've been mulling the idea of SpannerStub* Sesson::operator->() but I don't know if we want to go down that path. It also doesn't work with the streaming calls where we're capturing the stub, although if Brad goes through with his plan to make Session refcounted we could capture the shared_ptr<Session> instead.
Anyways, I left this as-is for now but I'll have a NYChove with Carlos.
google/cloud/spanner/internal/session.h, line 47 at r1 (raw file):
Previously, devbww (Bradley White) wrote…
move(stub)?
Done.
google/cloud/spanner/internal/session_pool.cc, line 145 at r1 (raw file):
Previously, coryan (Carlos O'Ryan) wrote…
+1 to that.
I think it's better here, because this is eventually going to be something like:
if (session->stub() == nullptr) {
// some logic to pick a stub from a set
// increment the count of sessions using that stub
session->set_stub(chosen_stub);
}
google/cloud/spanner/internal/session_pool_test.cc, line 244 at r1 (raw file):
Previously, devbww (Bradley White) wrote…
What does this replay purport to test?
Just that the stub hasn't changed across calls.
instead of permanently assigning a stub to a session that was born of a partition, just return one that can be used (since the call should only happen once).
There was a problem hiding this comment.
Reviewable status: 1 of 9 files reviewed, 6 unresolved discussions (waiting on @coryan, @devbww, @devjgm, and @scotthart)
google/cloud/spanner/internal/connection_impl.cc, line 296 at r1 (raw file):
Previously, mr-salty (Todd Derr) wrote…
I like this idea but I think it ends up being a little weird because of the in/out nature of the
SessionHolderand it not being copyable.
I split GetStub to another call so we can get it closer to where it's used... I'm ambivalent about whether the original function should live here or in SessionPool so I'm inclined to leave it here for now unless you feel strongly.
google/cloud/spanner/internal/connection_impl.cc, line 518 at r1 (raw file):
Previously, mr-salty (Todd Derr) wrote…
I think Brad was saying that if I changed
stub()to return a reference to theshared_ptrinstead of returning it by value (which seems to be the only way to prevent this extra refcount bump/decrement - is that something we really care about avoiding?), then aclear_stub()call (or anotherset_stub()) would be a problem.In reality returning a reference would still be "safe" because there is no
clear_stub()andset_stub()will only be called once, before any call tostub(). Although, I'd argue that returning a reference to a smart pointer is inherently an unsafe API. I really didn't want to haveset_stub()at all, then it's clear the stub is set in the constructor and never changes. bww and I tossed around some ideas about that yesterday, but playing around with them they seemed strictly worse, requiring adding extra methods on Connection or extra members to *Params structs. The way it's done in this PR effectively keeps all of that contained ininternalnamespace things, at the expense of awkward (internal) APIs like this.I still feel like there is probably a better solution, but also felt like I spun my wheels on this long enough. Carlos' suggestion above about returning the stub from a
SessionPoolmethod would be nice because I could then make the stub-related methods onSessionprivate withSessionPoolas a friend. But, it does always bump the reference count.I've been mulling the idea of
SpannerStub* Sesson::operator->()but I don't know if we want to go down that path. It also doesn't work with the streaming calls where we're capturing the stub, although if Brad goes through with his plan to makeSessionrefcounted we could capture theshared_ptr<Session>instead.Anyways, I left this as-is for now but I'll have a NYChove with Carlos.
ok, after discussing it with the team, Session::stub_ is now const but can still be nullptr. However, the only way to obtain the stub is by calling SessionPool::GetStub(session) which returns a SpannerStub for one-time use.
There was a problem hiding this comment.
Reviewed 9 of 10 files at r2.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @devbww, @devjgm, and @scotthart)
| @@ -44,7 +45,11 @@ class Session { | |||
| std::string const& session_name() const { return session_name_; } | |||
|
|
|||
| private: | |||
| std::string session_name_; | |||
| friend class SessionPool; // for access to stub() | |||
There was a problem hiding this comment.
PS: Is it worth it to try to hide stub()?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @devbww, @devjgm, and @scotthart)
google/cloud/spanner/internal/session.h, line 48 at r2 (raw file):
Previously, devbww (Bradley White) wrote…
PS: Is it worth it to try to hide stub()?
I wanted to force callers to use SessionPool::GetStub() instead of calling this. As we discussed in chat I think I should also make the constructor private, but I'll do that in a separate PR.
…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
SessionPoolmanages assigning aSpannerStubto eachSessionConnectionImplobtains the stub to use for a call from theSessionand no longer retains any knowledge of the stub.
This will enable the use of multiple stubs per
Connection; it'simportant for a
Sessionto remain associated with the stub/channelthat created it, otherwise there is a performance penalty.
Part of #307
This change is