core: LoadBalancer2 and DelayedClientTransport.reprocess(). #2443

zhangkun83 · 2016-11-24T02:27:43Z

This is the second step of a major refactor for the LoadBalancer-related
part of Channel impl (#1600).

Please review the PR as an entirety.

This is the second step of a major refactor for the LoadBalancer-related part of Channel impl (grpc#1600)

lukaszx0

Hate to bring up naming again, but I'd like to suggest renaming Picker to Selector. Maybe it's just me, but Picker always sounded weird for me in this context whereas Selector make it's more intuitive and gives me a better idea of what it'll be responsible for. Proposed new naming: SubchannelSelector, SelectedSubchanel, #selectSubchannel, #updateSubchannelSelector).

lukaszx0 · 2016-11-25T07:06:20Z

core/src/main/java/io/grpc/LoadBalancer2.java

+     * <p>The LoadBalancer is responsible for closing unused Subchannels, and closing all
+     * Subchannels within {@link #shutdown}.
+     */
+    public abstract Subchannel createSubChannel(EquivalentAddressGroup addrs, Attributes attrs);


nit: createSubChannel => createSubchannel to make it consistent with other class names

lukaszx0 · 2016-11-25T07:15:34Z

core/src/main/java/io/grpc/LoadBalancer2.java

+     * Schedule a task to be run in the Channel Executor, which serializes the task with the
+     * callback methods on the {@link LoadBalancer2} interface.
+     */
+    public abstract void runSeralized(Runnable task);


typo: runSerialized

lukaszx0 · 2016-11-25T07:36:33Z

core/src/main/java/io/grpc/LoadBalancer2.java

+    /**
+     * No decision could be made.  The RPC will stay buffered.
+     */
+    public static PickResult withNothingYet() {


Maybe withNoSubchannel and also updated comment that the returned result is successful (Status.OK) ?

LoadBalancer implementations care more about what constructor method to call, than what getStatus() returns. I think getStatus() is sufficiently documented, and it's unnecessary to talk about the resulting status.

I was struggling in the naming of this method. The message that I wanted to convey is that "LoadBalancer cannot make a decision yet, thus nothing will happen to this RPC.". I don't think "withNoSubchannel" does the job better. Some alternatives are:

withNoResult()

stillPending()

pending()

That makes sense. Maybe withNoResult() would work better? I like pending() the most, but it's missing with and I'd like to to be consistent with other methods. Heh.

Renamed to withNoResult().

zhangkun83 · 2016-11-25T20:48:43Z

I don't find Picker to be less intuitive than Selector, maybe because that we use "Picker" in our internal LB interface :). Given that, I do prefer Picker because it's shorter.

lukaszx0 · 2016-11-25T20:50:13Z

Sounds good. It's good enough so I won't be fighting over it ;)

On Fri, Nov 25, 2016 at 12:48 PM Kun Zhang ***@***.***> wrote: I don't find Picker to be less intuitive than Selector, maybe because that we use "Picker" in our internal LB interface :). Given that, I do prefer Picker because it's shorter. — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#2443 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAtKnVUKuqtkOI-WGceir8ulvbI2fkOks5rB0mxgaJpZM4K7OL5> .

-- Łukasz (on mobile)

lukaszx0 · 2016-11-26T07:02:50Z

core/src/main/java/io/grpc/LoadBalancer2.java

+    public abstract EquivalentAddressGroup getAddresses();
+
+    /**
+     * The same attributes passed to {@link #createSubChannel}. LoadBalancer can use it to attach


nit: Helper#createSubchannel

lukaszx0 · 2016-11-26T07:19:41Z

core/src/main/java/io/grpc/LoadBalancer2.java

+     * called again and a new picker replaces the old one.  If {@link #updatePicker} has never been
+     * called, the channel will buffer all RPCs until a picker is provided.
+     */
+    public abstract void updatePicker(SubchannelPicker picker);


Maybe rename to updateSubchannelPicker ?

I don't think it's necessary. updatePicker can't be mistaken for anything else in this context, given that the argument type already refers to "Subchannel".

lukaszx0 · 2016-11-26T19:29:38Z

core/src/main/java/io/grpc/LoadBalancer2.java

+     * additional information here, e.g., the shard this Subchannel belongs to.
+     */
+    public abstract Attributes getAttributes();
+  }


Attributes are immutable which means that if we want to be able to update attributes on existing channel, we'll need setAttributes(Attributes)

Not necessarily. You could use mutable values, e.g., AtomicReference in Attributes.

Ah yeah. That's fair.

lukaszx0

Looks good!

ejona86 · 2016-11-29T17:46:21Z

core/src/main/java/io/grpc/LoadBalancer2.java

+ *
+ * <h3>Channel Executor</h3>
+ *
+ * <p>Channel Executor is an internal executor of the channel, which is used to run serialize all


s/run serialize/serialize/ or similar.

ejona86 · 2016-11-29T17:47:31Z

core/src/main/java/io/grpc/LoadBalancer2.java

+ * <p>Channel Executor is an internal executor of the channel, which is used to run serialize all
+ * the callback methods on the {@link LoadBalancer2} interface, thus the balancer implementation
+ * doesn't need to worry about synchronization among them.  However, the actual thread that Channel
+ * Executor is typically the network thread, thus following rules must be followed to prevent


s/is/is is/ or s/actual thread that/actual thread of the/

ejona86 · 2016-11-29T17:48:43Z

core/src/main/java/io/grpc/LoadBalancer2.java

+ *   synchronization primitives, blocking I/O, blocking RPCs, etc.</li>
+ *
+ *   <li><strong>Avoid calling into other components with lock held</strong>.  Channel Executor may
+ *   run callbacks under a lock, e.g., the transport lock of OkHttp2.  If your LoadBalancer has a


s/OkHttp2/OkHttp/

ejona86 · 2016-11-29T17:52:10Z

core/src/main/java/io/grpc/LoadBalancer2.java

+
+  /**
+   * The main balancing logic.  It <strong>must be thread-safe</strong>. Typically it should only
+   * synchronize on its own state, and void synchronzing with the LoadBalancer's state.


s/void/avoid/

ejona86 · 2016-11-29T17:54:52Z

core/src/main/java/io/grpc/LoadBalancer2.java

+
+    /**
+     * A decision to proceed the RPC on a Subchannel.  The state of the Subchannel is supposed to be
+     * {@link ConnectivityState#READY}.  However, a non-READY Subchannel will not fail the RPC,


It was unclear why this "However" was stated. Maybe something like, "However, since such decisions are racy, a non-READY Subchannel..."

Exactly. Rephrased as suggested.

ejona86 · 2016-11-29T17:57:58Z

core/src/main/java/io/grpc/LoadBalancer2.java

+     * <p>When a new picker is provided via {@link Helper#updatePicker}, the channel will apply the
+     * picker on all buffered RPCs, by calling {@link SubchannelPicker#pickSubchannel}.
+     *
+     * <p>The channel will hold the a picker and use it for all RPCs, until {@link #updatePicker} is


s/the a/the/

ejona86 · 2016-11-29T18:00:33Z

core/src/main/java/io/grpc/LoadBalancer2.java

+   * <p>It maintains at most one physical connection (aka transport) for sending new RPCs, while
+   * also keeps track of previous transports that has been shut down but not terminated yet.
+   *
+   * <p>If there isn't an active transport yet, and the Subchannel is assigned to an RPC, it will


s/the Subchannel is assigned to an RPC/an RPC is assigned to the Subchannel/

ejona86 · 2016-11-29T19:05:11Z

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

+  // To be implemented by ManagedChannelImpl2 and unit test
+  @Nullable
+  ClientTransport obtainActiveTransport(Subchannel subchannel) {
+    throw new UnsupportedOperationException("Not implemented");


This seems strange, but I'm willing to wait and see how it all fits together.

Fair enough. I introduced a SubchannelImpl that adds obtainActiveTransport() to Subchannel.

ejona86 · 2016-11-29T19:12:31Z

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

+      }
+      pendingStreams.removeAll(toRemove);
+      if (pendingStreams.isEmpty()) {
+        pendingStreams = null;


I think pendingStreams should only be made null if the delayed transport is shut down and setTransport is called. Just leave the collection in-place, even if its empty.

Since setTransport() will go away, and delayed transport will be kept open, it seems necessary to take opportunity to discard the hashmap, as it only grows in size.

But the code doesn't ever create a new hashmap. The next stream that needs to be pending will cause a NPE. If you want to re-initialize, you'll need to create the new hashmap here.

Hmmm... do you also need to handle transport terminated here, like in PendingStream.cancel?

Good point! Actually I missed out the shutdown process for the LBv2 design. Would the following shutdown sequence work?:

ManagedChannelImpl.shutdown(): stop accepting new calls, shutdown delayed transport. All other functionalities continue working.

Once the delayed transport is terminated, shutdown NameResolver and LoadBalancer.

Once all subchannels and all OOB channels have terminated, ManagedChannelImpl will terminate.

Added termination handling, and fixed the NPE. Both covered by tests. PTAL.

ejona86 · 2016-11-29T19:13:33Z

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

+    }
+
+    synchronized (lock) {
+      if (pendingStreams == null || pendingStreams.isEmpty()) {


This is a bit worrying. If streams may be removed from pendingStreams, then that seems like we could "start" a stream more than once (maybe as failures).

I have added documentation to say that reprocess() cannot be called concurrently with itself or setTransport. However, between the two synchronized blocks, streams may be cancelled, and shutdown() may also be called, which will make pendingStreams empty and null respectively. Looks I have to keep this check.

zhangkun83

@ejona86 PTAL

zhangkun83 · 2016-11-30T00:16:41Z

core/src/main/java/io/grpc/LoadBalancer2.java

+ *
+ * <h3>Channel Executor</h3>
+ *
+ * <p>Channel Executor is an internal executor of the channel, which is used to run serialize all


zhangkun83 · 2016-11-30T00:17:39Z

core/src/main/java/io/grpc/LoadBalancer2.java

+ * <p>Channel Executor is an internal executor of the channel, which is used to run serialize all
+ * the callback methods on the {@link LoadBalancer2} interface, thus the balancer implementation
+ * doesn't need to worry about synchronization among them.  However, the actual thread that Channel
+ * Executor is typically the network thread, thus following rules must be followed to prevent


zhangkun83 · 2016-11-30T00:17:58Z

core/src/main/java/io/grpc/LoadBalancer2.java

+ *   synchronization primitives, blocking I/O, blocking RPCs, etc.</li>
+ *
+ *   <li><strong>Avoid calling into other components with lock held</strong>.  Channel Executor may
+ *   run callbacks under a lock, e.g., the transport lock of OkHttp2.  If your LoadBalancer has a


zhangkun83 · 2016-11-30T00:18:20Z

core/src/main/java/io/grpc/LoadBalancer2.java

+
+  /**
+   * The main balancing logic.  It <strong>must be thread-safe</strong>. Typically it should only
+   * synchronize on its own state, and void synchronzing with the LoadBalancer's state.


zhangkun83 · 2016-11-30T00:19:23Z

core/src/main/java/io/grpc/LoadBalancer2.java

+
+    /**
+     * A decision to proceed the RPC on a Subchannel.  The state of the Subchannel is supposed to be
+     * {@link ConnectivityState#READY}.  However, a non-READY Subchannel will not fail the RPC,


Exactly. Rephrased as suggested.

zhangkun83 · 2016-11-30T00:19:59Z

core/src/main/java/io/grpc/LoadBalancer2.java

+     * <p>When a new picker is provided via {@link Helper#updatePicker}, the channel will apply the
+     * picker on all buffered RPCs, by calling {@link SubchannelPicker#pickSubchannel}.
+     *
+     * <p>The channel will hold the a picker and use it for all RPCs, until {@link #updatePicker} is


zhangkun83 · 2016-11-30T00:21:29Z

core/src/main/java/io/grpc/LoadBalancer2.java

+   * <p>It maintains at most one physical connection (aka transport) for sending new RPCs, while
+   * also keeps track of previous transports that has been shut down but not terminated yet.
+   *
+   * <p>If there isn't an active transport yet, and the Subchannel is assigned to an RPC, it will


zhangkun83 · 2016-11-30T00:32:07Z

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

+      }
+      pendingStreams.removeAll(toRemove);
+      if (pendingStreams.isEmpty()) {
+        pendingStreams = null;


Since setTransport() will go away, and delayed transport will be kept open, it seems necessary to take opportunity to discard the hashmap, as it only grows in size.

zhangkun83 · 2016-11-30T01:21:27Z

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

+  // To be implemented by ManagedChannelImpl2 and unit test
+  @Nullable
+  ClientTransport obtainActiveTransport(Subchannel subchannel) {
+    throw new UnsupportedOperationException("Not implemented");


Fair enough. I introduced a SubchannelImpl that adds obtainActiveTransport() to Subchannel.

zhangkun83 · 2016-11-30T01:39:17Z

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

+    }
+
+    synchronized (lock) {
+      if (pendingStreams == null || pendingStreams.isEmpty()) {


I have added documentation to say that reprocess() cannot be called concurrently with itself or setTransport. However, between the two synchronized blocks, streams may be cancelled, and shutdown() may also be called, which will make pendingStreams empty and null respectively. Looks I have to keep this check.

zhangkun83 added 5 commits November 22, 2016 22:44

LoadBalancer2 and reprocess() in delayed transport.

652a0ef

This is the second step of a major refactor for the LoadBalancer-related part of Channel impl (grpc#1600)

More comprehensive test.

3292c60

More test coverage.

58bcc00

Prune an unused branch.

18a5252

The new LoadBalancer interface, complete javadocs.

aefcd52

zhangkun83 assigned lukaszx0 and ejona86 Nov 24, 2016

lukaszx0 reviewed Nov 25, 2016

View reviewed changes

lukaszx0 reviewed Nov 26, 2016

View reviewed changes

zhangkun83 added 2 commits November 28, 2016 11:02

Fix typos and rename withNothingYet->withNoResult.

fb492aa

Add tests for withNoResult().

feee7c0

lukaszx0 approved these changes Nov 28, 2016

View reviewed changes

ejona86 reviewed Nov 29, 2016

View reviewed changes

zhangkun83 added 3 commits November 29, 2016 16:22

Fix grammars and typos.

a020089

SubchannelImpl supercedes the strange override.

aa498cb

Tighten the threading requirement of reprocess().

db88964

zhangkun83 commented Nov 30, 2016

View reviewed changes

zhangkun83 added 2 commits November 30, 2016 15:50

Re-create pendingStreams instead of setting it to null.

b54002f

Terminate delayed transport in reprocess() when appropriate.

deb0d56

ejona86 approved these changes Dec 2, 2016

View reviewed changes

zhangkun83 merged commit 7ec8167 into grpc:master Dec 2, 2016

zhangkun83 deleted the lbv2_delayedtransport branch December 2, 2016 18:35

lock bot locked as resolved and limited conversation to collaborators Jan 21, 2019

core: LoadBalancer2 and DelayedClientTransport.reprocess(). #2443

core: LoadBalancer2 and DelayedClientTransport.reprocess(). #2443

Conversation

zhangkun83 commented Nov 24, 2016

lukaszx0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhangkun83 Nov 28, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhangkun83 commented Nov 25, 2016

lukaszx0 commented Nov 25, 2016 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukaszx0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhangkun83 Dec 1, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhangkun83 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhangkun83 Nov 28, 2016 •

edited

Loading

zhangkun83 Dec 1, 2016 •

edited

Loading