api: pass Subchannel state updates to SubchannelStateListener rather than LoadBalancer (take 2) #5722

zhangkun83 · 2019-05-10T06:36:40Z

This is a revised version of #5503 (62b03fd), which was rolled back in f8d0868. The newer version passes SubchannelStateListener to Subchannel.start() instead of SubchannelCreationArgs, which allows us to remove the Subchannel argument from the listener, which works as a solution for #5676.

LoadBalancers that call the old createSubchannel() will get start() implicitly called with a listener that passes updates to the deprecated LoadBalancer.handleSubchannelState(). Those who call the new createSubchannel() will have to call start() explicitly.

This PR has 3 commits:

Roll forward the original change.
Revert GRPCLB back to the old API. See commit message for the reason.
The revision.

Nothing new is introduced in commits 1 and 2. Reviewers should only need to review commit 3.

This reverts commit f8d0868.

It's a pain to migrate the SubchannelPool to the new API. Since CachedSubchannelHelper is on the way, it's easier to switch to it when it's ready. Keeping GRPCLB with the old API would also confirm the backward compatibility.

Also stop passing Subchannel to SubchannelStateListener. This is part of the bigger change to remove Subchannel from all arguments on the LoadBalancer API, so that wrapping of Subchannel won't cause identity problems. Details in grpc#5676

zhangkun83 · 2019-05-10T16:59:40Z

There is a caveat, that if the parent LB and the child LB are using different versions of APIs, the parent won't be able to intercept Subchannel states, like what HealthCheckingLoadBalancerFactory is doing.

Parent LB is on the old API and the child is on the new: child passes listener to Subchannel.start(), which the parent is not intercepting, because the parent is intercepting LoadBalancer.handleSubchannelState(). Subchannel state updates go to the child listener intact.
Parent LB is on the new API and the child is on the old: child calls the old createSubchannel() which the parent is not intercepting, because the parent is intercepting the new createSubchannel() and returning a wrapped Subchannel that will intercept start(). ChannelImpl converts the old createSubchannel() into a new one followed by an implicit start(), which calls handleSubchannelState() on the parent which is unintercepted. Subchannel state updates go to the child listener intact.
When parent and child use the same version, Subchannel states get correctly intercepted.

I can't think of a way to perfectly solve this. Hopefully and probably nobody other than us is intercepting Subchannel states.

ejona86 · 2019-05-14T14:33:26Z

api/src/main/java/io/grpc/LoadBalancer.java

+     * Starts the Subchannel.  Can only be called once.
+     *
+     * <p>This method <strong>must be called from the {@link #getSynchronizationContext
+     * Synchronization Context}</strong>, otherwise it may throw. This is to avoid the race between


I think this comment is now a bit out-of-date. For example the race mentioned in "This is to avoid the race ..." is no longer a problem for the caller.

ejona86 · 2019-05-14T14:34:58Z

api/src/main/java/io/grpc/LoadBalancer.java


    /**
     * Asks the Subchannel to create a connection (aka transport), if there isn't an active one.
+     * Has no effect if {@link #start} has not been called or {@link #shutdown} has been called.


I think it should throw if start has not been called. I'd be willing to have a blanket statement for the class that all methods may throw if called before start. We could look at ClientCall for seeing how we worded it there.

I just realized we can't make the sync-context requirement for this method, because pick-first calls it from the data-path. I have sent out #5736

Per our discussion offline, we will keep the sync-context requirement, and all methods except shutdown() should be called after start().

ejona86 · 2019-05-14T21:50:46Z

core/src/main/java/io/grpc/internal/ManagedChannelImpl.java


    @Override
    public void requestConnection() {
+      syncContext.execute(new Runnable() {


Why was this changed? It seems it should be safe to check started here without going onto the syncContext.

Reverted per our discussion. There is a happens-after relationship between start() and the other methods, thus started don't need to be read in the syncContext.

core/src/main/java/io/grpc/util/RoundRobinLoadBalancer.java

voidzcy · 2019-05-14T23:48:37Z

services/src/main/java/io/grpc/services/HealthCheckingLoadBalancerFactory.java

    }

-    void updateRawState(ConnectivityStateInfo rawState) {
+    public void onSubchannelState(ConnectivityStateInfo rawState) {


Isn't HealthCheckState implementing SubchannelStateListener any more?

You are right. Made HealthCheckState implement SubchannelStateListener again.

…stener2

api/src/test/java/io/grpc/ForwardingTestUtil.java

dapengzhang0 · 2019-05-15T17:56:00Z

core/src/main/java/io/grpc/internal/ManagedChannelImpl.java

  }

+  // Must be called from syncContext
+  private void handleInternalSubchannelState(ConnectivityStateInfo newState) {


You don't need to move this method, because you can access the LbHelperImpl field.

This method doesn't need to be on LbHelperImpl in the first place. What it does is in the scope of the top-level class.

dapengzhang0 · 2019-05-15T18:00:06Z

core/src/main/java/io/grpc/internal/ManagedChannelImpl.java

+        });
+    }
+
+    private void internalShutdown() {


This method is only used in one place. Consider move it to a named inner class?

This method is intended to replace shutdown() when we are ready to turn the warning into an exception. I have added a comment to make that clear.

dapengzhang0 · 2019-05-15T18:00:35Z

core/src/main/java/io/grpc/internal/ManagedChannelImpl.java

+    }
+
+    private void internalShutdown() {
+      syncContext.throwIfNotInThisSynchronizationContext();


It's obvious in syncContext already.

Please see my response above.

core/src/main/java/io/grpc/internal/ManagedChannelImpl.java

dapengzhang0 · 2019-05-15T19:06:55Z

core/src/main/java/io/grpc/internal/ManagedChannelImpl.java

+        syncContext.execute(new Runnable() {
+            @Override
+            public void run() {
+              listener.onSubchannelState(ConnectivityStateInfo.forNonError(SHUTDOWN));


The javadoc says SHUTDOWN can be safely ignored, do we need call onSubchannelState(SHUTDOWN)?

The paragraph where this statement is from is the continuation of the previous paragraph, which talks about the need to create a picker for each update. Although a LoadBalancer usually don't need to react to a SHUTDOWN update, the ChannelImpl still need to propagate it for completeness. And in some cases e.g., HealthCheckingLoadBalancer and CachedSubchannelHelper, the SHUTDOWN update is actually consumed for cleanups. I have rephrased the javadoc to prevent confusion.

requestConnection() in sync-context, because it's not necessary.

zhangkun83 · 2019-05-15T21:09:25Z

@ejona86 @dapengzhang0 @voidzcy thanks for the review. PTAL.

We will require Subchannel.requestConnection() to be called from sync-context (grpc#5722), but SubchannelPicker.requestConnection() is currently calling it with the assumption of thread-safety. Actually SubchannelPicker.requestConnection() is called already from sync-context by ChannelImpl, it makes more sense to move this method to LoadBalancer where all other methods are sync-context'ed, rather than making SubchannelPicker.requestConnection() sync-context'ed and fragmenting the SubchannelPicker API because pickSubchannel() is thread-safe. C++ also has the requestConnection() equivalent on their LoadBalancer interface.

We will require Subchannel.requestConnection() to be called from sync-context (#5722), but SubchannelPicker.requestConnection() is currently calling it with the assumption of thread-safety. Actually SubchannelPicker.requestConnection() is called already from sync-context by ChannelImpl, it makes more sense to move this method to LoadBalancer where all other methods are sync-context'ed, rather than making SubchannelPicker.requestConnection() sync-context'ed and fragmenting the SubchannelPicker API because pickSubchannel() is thread-safe. C++ also has the requestConnection() equivalent on their LoadBalancer interface.

zhangkun83 added 2 commits May 9, 2019 13:47

core: re-introduce SubchannelStateListener.

dcc246b

This reverts commit f8d0868.

Revert GRPCLB back to the deprecated Subchannel state API.

579e2a6

It's a pain to migrate the SubchannelPool to the new API. Since CachedSubchannelHelper is on the way, it's easier to switch to it when it's ready. Keeping GRPCLB with the old API would also confirm the backward compatibility.

zhangkun83 requested a review from ejona86 May 10, 2019 06:36

zhangkun83 force-pushed the subchannelstatelistener2 branch from b3a9b3e to 98710a3 Compare May 10, 2019 07:04

zhangkun83 requested review from dapengzhang0 and voidzcy May 14, 2019 17:11

ejona86 approved these changes May 14, 2019

View reviewed changes

voidzcy reviewed May 14, 2019

View reviewed changes

zhangkun83 added 4 commits May 14, 2019 17:09

Update comments

b8d6966

Set shutdown bit when not started

4083345

Merge remote-tracking branch 'upstream/master' into subchannelstateli…

8d94fc7

…stener2

Fix nnnnns

9923f02

dapengzhang0 reviewed May 15, 2019

View reviewed changes

zhangkun83 added 4 commits May 15, 2019 12:21

Methods can be called after shutdown(). stop scheduling

980ece6

requestConnection() in sync-context, because it's not necessary.

Fix javadoc

d4b369f

Make HealthCheckState implement SubchannelStateListener

8ef8f49

Address comments

fd82ff9

AssertionError

3454dbc

zhangkun83 mentioned this pull request May 16, 2019

api: move SubchannelPicker.requestConnection() to LoadBalancer. #5751

Merged

zhangkun83 added 3 commits May 16, 2019 10:48

Merge branch 'master' into subchannelstatelistener2

517e361

Suppress deprecation warning in xds test

c2f5074

Merge branch 'master' into subchannelstatelistener2

f6a20e7

voidzcy self-requested a review May 17, 2019 23:03

voidzcy approved these changes May 17, 2019

View reviewed changes

dapengzhang0 approved these changes May 17, 2019

View reviewed changes

zhangkun83 merged commit 7934594 into grpc:master May 17, 2019

zhangkun83 deleted the subchannelstatelistener2 branch May 17, 2019 23:37

zhangkun83 mentioned this pull request Jun 7, 2019

Extract name-resolution logic from ManagedChannelImpl for re-use #5496

Open

lock bot locked as resolved and limited conversation to collaborators Aug 15, 2019

api: pass Subchannel state updates to SubchannelStateListener rather than LoadBalancer (take 2) #5722

api: pass Subchannel state updates to SubchannelStateListener rather than LoadBalancer (take 2) #5722

Uh oh!

Conversation

zhangkun83 commented May 10, 2019

Uh oh!

zhangkun83 commented May 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhangkun83 commented May 15, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

zhangkun83 commented May 10, 2019 •

edited

Loading