cache: respond to watches in parallel v2 #85

sschepens · 2018-11-30T14:51:20Z

Supersedes #75

Added concept of ExecutorGroup mostly based on netty's EventExecutorGroup for distributing Executors to streams in a round robin fashion.

sschepens · 2018-11-30T15:06:19Z

tests are failing on circle but work ok on my computer, investigating

codecov-io · 2018-11-30T15:17:35Z

Codecov Report

Merging #85 into master will increase coverage by 0.08%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master      #85      +/-   ##
============================================
+ Coverage     94.39%   94.47%   +0.08%     
- Complexity      131      134       +3     
============================================
  Files            13       14       +1     
  Lines           517      525       +8     
  Branches         46       46              
============================================
+ Hits            488      496       +8     
  Misses           21       21              
  Partials          8        8

Impacted Files	Coverage Δ	Complexity Δ
...nvoyproxy/controlplane/server/DiscoveryServer.java	`96.49% <100%> (+0.19%)`	`14 <1> (+1)`	⬆️
...roxy/controlplane/server/DefaultExecutorGroup.java	`100% <100%> (ø)`	`2 <2> (?)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c56c848...0743dee. Read the comment docs.

snowp · 2018-12-02T19:45:32Z

cache/src/main/java/io/envoyproxy/controlplane/cache/SimpleCache.java

@@ -52,7 +53,18 @@
   * @param groups maps an envoy host to a node group
   */
  public SimpleCache(NodeGroup<T> groups) {
+    this (groups, new DefaultExecutorGroup());
+  }
+


nit: extra whitespace

snowp · 2018-12-02T19:46:07Z

cache/src/main/java/io/envoyproxy/controlplane/cache/Watch.java

+   *
+   */
+  public Watch(boolean ads, DiscoveryRequest request, Consumer<Response> responseConsumer) {
+    this(ads, request, responseConsumer, MoreExecutors.directExecutor());


please document the default behavior

snowp · 2018-12-02T19:46:37Z

cache/src/test/java/io/envoyproxy/controlplane/cache/SimpleCacheTest.java

@@ -92,12 +94,14 @@ public void invalidNamesListShouldReturnWatcherWithResponseInXdsMode() {
        Collections.emptySet(),
        responseTracker);

+    Thread.yield();


is this necessary?

snowp · 2018-12-02T19:51:03Z

cache/src/main/java/io/envoyproxy/controlplane/cache/DefaultExecutorGroup.java

+ * children them in a round robin fashion.
+ *
+ */
+public class DefaultExecutorGroup implements ExecutorGroup {


Could you improve the test coverage of this class?

snowp · 2018-12-02T19:52:35Z

cache/src/main/java/io/envoyproxy/controlplane/cache/DefaultExecutorGroup.java

+      try {
+        children[i] = Executors.newSingleThreadExecutor();
+        success = true;
+      } catch (Exception e) {


When do you expect this to throw? I'd prefer to avoid unnecessary error handling if possible

snowp · 2018-12-02T19:58:00Z

cache/src/main/java/io/envoyproxy/controlplane/cache/DefaultExecutorGroup.java

+import java.util.stream.Collectors;
+
+/**
+ * Default implementation of {@link ExecutorGroup} which defaults to


The only thing that ties this implementation to the number of processors is the default ctor, so I'd rephrase this to describe this just as a fixed size RR implementation and move the bit about number of processors to the default ctor.

snowp · 2018-12-02T20:11:03Z

cache/src/main/java/io/envoyproxy/controlplane/cache/SimpleCache.java

@@ -102,7 +114,7 @@ public Watch createWatch(
      Snapshot snapshot = snapshots.get(group);
      String version = snapshot == null ? "" : snapshot.version(request.getTypeUrl());

-      Watch watch = new Watch(ads, request, responseConsumer);
+      Watch watch = new Watch(ads, request, responseConsumer, executorGroup.next());


by RRing thread assignments to individual watches like this we're not gonna get thread affinity per stream which imo is a minimum requirement for this to make sense for ADS. We probably want to select the executor per stream, not per watch

If possible, I would love a test that ensures that the ordering of updates sent with ADS is correct, but that might be out of the scope of this PR.

I was under the impression we created only one watch per stream, but this is only valid when not using ADS, we're probably going to need to move the RR to DiscoveryServer

snowp · 2018-12-02T20:11:15Z

cache/src/main/java/io/envoyproxy/controlplane/cache/SimpleCache.java

+  /**
+   * Constructs a simple cache.
+   *
+   * @param groups maps an envoy host to a node group


document parameter please

snowp · 2018-12-02T20:19:36Z

cache/src/main/java/io/envoyproxy/controlplane/cache/ExecutorGroup.java

+ * life-cycle and allows shutting them down in a global fashion.
+ *
+ */
+public interface ExecutorGroup extends ExecutorService {


You could probably simplify this a lot by not extending ExecutorService. You can provide a terminate/shutdown/etc on the DefaultExecutorGroup directly if you feel like that's necessary

snowp · 2018-12-02T20:24:07Z

cache/src/main/java/io/envoyproxy/controlplane/cache/DefaultExecutorGroup.java

+   *
+   * @param nThreads amount of children threads
+   */
+  public DefaultExecutorGroup(int nThreads) {


something like threadCount would probably be more consistent with parameter naming used elsewhere in the project

sschepens · 2018-12-03T14:33:21Z

@snowp DefaultExecutorGroup is mostly copypasted from Netty DefaultEventExecutorGroup, wouldn't you think it's probably better to add a dependency on netty-common and use that?
We would avoid having to test and mantain this class.

snowp · 2018-12-03T15:29:52Z

I would actually go in the other direction: provide the minimum interface necessary here and a very simple implementation, maybe something that just returns the same fixed thread executor each time. That way you can use a netty implementation if you want without bringing all that complexity into the control plane API.

Using a netty implementation would be as simple as doing

  class NettyExecutorGroup implements ExecutorGroup {
    EventExecutorGroup nettyGroup;
    
    @Override public Executor next() {
      return nettyGroup.next();
    }
  }

sschepens · 2018-12-03T19:09:26Z

@snowp thanks for the comments, totally agree.

I committed a different, simpler approach, moved ExecutorGroup and DefaultExecutorGroup to server to RR Executors per stream.
Also DefaultExecutorGroup now defaults to the old behavior by just returning directExecutor.

This approach is much simpler and touches less things, please review.

Signed-off-by: Sebastian Schepens <sebastian.schepens@mercadolibre.com>

snowp

Looks pretty good! Can you update the PR description to reflect the new approach?

snowp · 2018-12-05T14:12:48Z

server/src/main/java/io/envoyproxy/controlplane/server/DefaultExecutorGroup.java

+  /**
+   * Creates a new instance.
+   */
+  public DefaultExecutorGroup() {


nit: no need for this default ctor

snowp · 2018-12-05T14:17:08Z

server/src/main/java/io/envoyproxy/controlplane/server/DefaultExecutorGroup.java

+
+/**
+ * Default implementation of {@link ExecutorGroup} which
+ * always returns {@link com.google.common.util.concurrent.MoreExecutors#directExecutor()}.


supernit: shorten this to reference MoreExecutors#directExecutor instead of the fqn

joeyb

Looks really good, mostly just some annoying nitpick comments.

We don't have to solve this now, but it would be good to document this a bit more. I get the mechanics of it from a high-level, but I don't think I have a full picture of the different types of ExecutorGroup impls that people would want in the real world. Are there use-cases where you would want certain request streams to have an affinity for a particular Executor? Or is it mostly just direct executor vs. thread pool, with the pool size and pool entry chooser (e.g. netty's PowerOfTwoEventExecutorChooser) being the most interesting parts of the latter?

joeyb · 2018-12-05T14:47:09Z

server/src/main/java/io/envoyproxy/controlplane/server/DefaultExecutorGroup.java

+  public DefaultExecutorGroup() {
+  }
+
+  @Override


/** * Returns the next {@link Executor} to use, which in this case is always {@link MoreExecutors#directExecutor()}. */

joeyb · 2018-12-05T14:51:24Z

server/src/main/java/io/envoyproxy/controlplane/server/DiscoveryServer.java

+   * @param configWatcher source of configuration updates
+   * @param executorGroup executor group to use for responding stream requests
+   */
+  public DiscoveryServer(List<DiscoveryServerCallbacks> callbacks, ConfigWatcher configWatcher,


nit: when wrapping parameters, prefer to have each on their own line, i.e.:

public DiscoveryServer( List<DiscoveryServerCallbacks> callbacks, ConfigWatcher configWatcher, ExecutorGroup executorGroup) { // ... }

Subjectively, it's easier to read quickly because it's more consistent. I still need to figure out whether or not it's possible to encode that in the checkstyle rules.

joeyb · 2018-12-05T14:54:17Z

server/src/main/java/io/envoyproxy/controlplane/server/ExecutorGroup.java

+ */
+public interface ExecutorGroup {
+  /**
+   * Return the next {@link Executor} to use.


nit: the google java style guide prefers Returns over Return here, and the extra javadoc line below can be removed.

joeyb · 2018-12-05T14:56:56Z

server/src/main/java/io/envoyproxy/controlplane/server/ExecutorGroup.java

+   * Return the next {@link Executor} to use.
+   *
+   */
+  Executor next();


Does it make sense to also pass some contextual information about the request stream here? Would there be use-cases where e.g. a particular type would need to have affinity to a specific Executor?

cc: @snowp

I thought about this as well, but there's not much contextual information we can send at the moment

I think having each stream always using the same executor is good enough for now

joeyb · 2018-12-05T14:58:14Z

server/src/main/java/io/envoyproxy/controlplane/server/DiscoveryServer.java


    private AtomicLong streamNonce;

    public DiscoveryRequestStreamObserver(String defaultTypeUrl, StreamObserver<DiscoveryResponse> responseObserver,
-                                          long streamId, boolean ads) {
+                                          long streamId, boolean ads, Executor executor) {


nit: ditto on the comment about parameter wrapping.

sschepens · 2018-12-05T15:48:08Z

We don't have to solve this now, but it would be good to document this a bit more. I get the mechanics of it from a high-level, but I don't think I have a full picture of the different types of ExecutorGroup impls that people would want in the real world. Are there use-cases where you would want certain request streams to have an affinity for a particular Executor? Or is it mostly just direct executor vs. thread pool, with the pool size and pool entry chooser (e.g. netty's PowerOfTwoEventExecutorChooser) being the most interesting parts of the latter?

@joeyb I think it would mostly be just direct executor vs. thread pool, What I can see is people probably trying to reuse netty's worker groups, I would personally like that.
I can see little benefit from having stream affinity, even so, right now we have little to no context for taking these decisions.

Signed-off-by: Sebastian Schepens <sebastian.schepens@mercadolibre.com>

joeyb · 2018-12-05T16:07:08Z

@sschepens - Cool, makes sense. If the need for more context comes up, then we can cross that bridge when we get there.

snowp

Thanks!

joeyb · 2018-12-09T23:08:59Z

@sschepens, @snowp - FYI, I cut 0.1.12, which includes this change.

sschepens · 2018-12-10T18:30:50Z

@joeyb thanks!

sschepens force-pushed the para2 branch 6 times, most recently from 9e5a708 to e913472 Compare November 30, 2018 15:02

sschepens force-pushed the para2 branch from e913472 to e46e3e3 Compare November 30, 2018 15:15

snowp requested changes Dec 2, 2018

View reviewed changes

sschepens force-pushed the para2 branch 2 times, most recently from 7482be5 to 622e463 Compare December 3, 2018 19:08

sschepens force-pushed the para2 branch 2 times, most recently from 4f3db1e to 2f9543a Compare December 3, 2018 19:17

Add ExecutorGroup to allow to respond watches in parallel

1977f72

Signed-off-by: Sebastian Schepens <sebastian.schepens@mercadolibre.com>

sschepens force-pushed the para2 branch from 2f9543a to 1977f72 Compare December 3, 2018 19:19

snowp requested changes Dec 5, 2018

View reviewed changes

joeyb reviewed Dec 5, 2018

View reviewed changes

sschepens force-pushed the para2 branch from 1d7b03b to fbd96b3 Compare December 5, 2018 15:45

Address comments

0743dee

Signed-off-by: Sebastian Schepens <sebastian.schepens@mercadolibre.com>

sschepens force-pushed the para2 branch from fbd96b3 to 0743dee Compare December 5, 2018 15:49

joeyb approved these changes Dec 5, 2018

View reviewed changes

snowp approved these changes Dec 8, 2018

View reviewed changes

snowp merged commit 771dd57 into envoyproxy:master Dec 8, 2018

This was referenced Dec 10, 2018

cache: respond to watches in parallel #75

Closed

Respond to watches in parallel #59

Closed

sschepens deleted the para2 branch December 10, 2018 18:30

cache: respond to watches in parallel v2 #85

cache: respond to watches in parallel v2 #85

Uh oh!

Conversation

sschepens commented Nov 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sschepens commented Nov 30, 2018

Uh oh!

codecov-io commented Nov 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

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!

sschepens commented Dec 3, 2018

Uh oh!

snowp commented Dec 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sschepens commented Dec 3, 2018

Uh oh!

snowp left a comment

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!

joeyb left a comment

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!

Choose a reason for hiding this comment

Uh oh!

sschepens commented Dec 5, 2018

Uh oh!

joeyb commented Dec 5, 2018

Uh oh!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

joeyb commented Dec 9, 2018

Uh oh!

sschepens commented Dec 10, 2018

Uh oh!

Uh oh!

sschepens commented Nov 30, 2018 •

edited

Loading

codecov-io commented Nov 30, 2018 •

edited

Loading

snowp commented Dec 3, 2018 •

edited

Loading