Connected subchannel refactoring#13932
Merged
dgquintas merged 9 commits intogrpc:masterfrom Jan 18, 2018
Merged
Conversation
This was referenced Jan 5, 2018
dgquintas
force-pushed
the
conn_subchannel
branch
from
64d82a8 to
32ac787
Compare
|
|
dgquintas
force-pushed
the
conn_subchannel
branch
from
32ac787 to
f9ea1df
Compare
|
|
|
|
dgquintas
force-pushed
the
conn_subchannel
branch
from
f9ea1df to
2906010
Compare
|
|
|
dgquintas
force-pushed
the
conn_subchannel
branch
from
2906010 to
53bfe69
Compare
|
|
dgquintas
commented
Jan 6, 2018
| #define GRPC_SUBCHANNEL_REF_EXTRA_ARGS | ||
| #endif | ||
|
|
||
| class grpc_connected_subchannel : public grpc_core::RefCountedWithTracing { |
Contributor
Author
There was a problem hiding this comment.
Yes, we may want to rename the class to follow C++ class naming conventions.
|
Member
|
This looks really good! Most of my comments are minor. Reviewed 10 of 10 files at r1. src/core/ext/filters/client_channel/lb_policy.h, line 163 at r1 (raw file):
Should we change this to return a RefCountedPtr? This might be easier once #13857 is merged. We should probably discuss which of these PRs we should try to merge first. src/core/ext/filters/client_channel/subchannel.cc, line 169 at r1 (raw file):
Shouldn't these two functions go away? src/core/ext/filters/client_channel/subchannel.cc, line 555 at r1 (raw file):
Instead of using a src/core/ext/filters/client_channel/subchannel.cc, line 773 at r1 (raw file):
Instead of taking and releasing refs to the channel stack every time we take and release a ref to the connected subchannel, why don't we just take a single ref to the channel stack in the ctor and release it in the dtor? This should ideally eliminate the need to override the src/core/ext/filters/client_channel/subchannel.h, line 51 at r1 (raw file):
These two macros should go away. src/core/ext/filters/client_channel/subchannel.h, line 68 at r1 (raw file):
Same here. src/core/ext/filters/client_channel/subchannel.h, line 76 at r1 (raw file): Previously, dgquintas (David G. Quintas) wrote…
Yup. Why not go ahead and do that in this PR? src/core/ext/filters/client_channel/subchannel.h, line 76 at r1 (raw file):
We should probably put this new code in the src/core/ext/filters/client_channel/subchannel.h, line 88 at r1 (raw file):
This should be src/core/ext/filters/client_channel/subchannel.h, line 98 at r1 (raw file):
The first arg could be a reference instead of a pointer. src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc, line 415 at r1 (raw file):
Seems very weird to use src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc, line 415 at r1 (raw file):
Why move this block up here from its oriignal position? I'm pretty sure this is unsafe, because if we unref the subchannel list, we will then try to access src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc, line 432 at r1 (raw file):
SHUTDOWN state should never happen here anymore. src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc, line 449 at r1 (raw file):
Might be a good idea to refactor src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc, line 457 at r1 (raw file):
This should never happen anymore. src/core/lib/debug/trace.h, line 91 at r1 (raw file):
The intent of this interface was that for non-debug builds, src/core/lib/support/ref_counted.h, line 101 at r1 (raw file):
We also need this in the (I had added this in #13684, but it's fine to do it here too, or just move this to its own PR if you want. Whichever one gets merged first will win.) src/core/lib/support/ref_counted.h, line 113 at r1 (raw file):
Why remove this? We generally want all destructors to be virtual for a base class. Comments from Reviewable |
Member
|
Review status: all files reviewed at latest revision, 18 unresolved discussions, some commit checks failed. src/core/ext/filters/client_channel/lb_policy.h, line 163 at r1 (raw file): Previously, markdroth (Mark D. Roth) wrote…
I've just merged #13857, so you'll have to merge the changes into this PR. src/core/lib/debug/trace.h, line 91 at r1 (raw file): Previously, markdroth (Mark D. Roth) wrote…
I think I'm running into this same problem in #13684. I'm going to try to come up with a better fix as part of that PR. Comments from Reviewable |
|
|
|
|
|
|
|
Member
|
This looks good to me. The only thing I'm waiting on to approve this is that I'd like to get #13984 merged first, and that's waiting on your review, so please take a look when you have a chance. Thanks! |
Contributor
Author
|
#13984 has been merged. Approval time? |
Member
|
Reviewed 1 of 1 files at r6, 3 of 4 files at r7. src/core/lib/support/ref_counted.h, line 104 at r7 (raw file):
Looks like you've got this macro here twice now. Comments from Reviewable |
Contributor
Author
|
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed. src/core/lib/support/ref_counted.h, line 104 at r7 (raw file): Previously, markdroth (Mark D. Roth) wrote…
Done. Comments from Reviewable |
|
|
|
|
|
|
Contributor
Author
|
Issues #13148 |
dgquintas
added a commit
that referenced
this pull request
Jan 26, 2018
Revert "Merge pull request #13932 from dgquintas/conn_subchannel"
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR makes connected subchannel into an actual object (while making it C++ while at it). This allows subchannels to keep their connectivity as TRANSIENT FAILURE even while the connected subchannel has lost connectivity. Subchannels can then attempt to recreate the connected subchannel, while their connectivity state subscribers (LB policies) are only aware of the TRANSIENT FAILURE status and may see an eventual transition to READY if the subchannel's reconnection attempt is successful. Previously, TRANSIENT FAILURE was a terminal state for subchannels (in fact, it was internally turned into SHUTDOWN), meaning subchannels wouldn't recover from disconnections, which resulted in the problems described in #11643.
Fixes #11643
This change is