Implement flush coalescing in OkHttp. #4763

creamsoup · 2018-08-15T17:54:47Z

Implementation is skipping flush task if another flush is queued.

Resolves #3258

ejona86

Note that checkstyle is failing in the CIs.

ejona86 · 2018-08-15T18:38:59Z

okhttp/src/test/java/io/grpc/okhttp/AsyncFrameWriterTest.java

+  @Before
+  public void setUp() throws Exception {
+    queueingExecutor = new QueueingExecutor();
+    asyncFrameWriter = spy(new AsyncFrameWriter(transport, new SerializingExecutor(queueingExecutor)));


Please, please no spies. They are so very magical, I don't trust tests with them.

spy is gone!

ejona86 · 2018-08-15T18:39:39Z

okhttp/src/test/java/io/grpc/okhttp/AsyncFrameWriterTest.java

+    queueingExecutor.runAll();
+
+    verify(frameWriter, times(1)).ping(anyBoolean(), anyInt(), anyInt());
+    verify(asyncFrameWriter, times(1)).flush();


What is the point of this verification, here and elsewhere? Isn't this test the thing that called this method, on line 56?

right. removed unnecessary verify.

ejona86 · 2018-08-15T18:43:39Z

okhttp/src/test/java/io/grpc/okhttp/AsyncFrameWriterTest.java

+
+  private static abstract class PartiallyTrackingFrameWriter implements FrameWriter {
+
+    private static final List<Method> methods = new ArrayList<Method>();


This is just asking for trouble, as one test can influence another. Actually... how is flushCoalescing_shouldMergeTwoQueuedFlushes working? Nothing is clearing out these methods.

Implementation is skipping flush task if another flush is queued.

carl-mastrangelo · 2018-08-15T18:56:41Z

okhttp/src/test/java/io/grpc/okhttp/AsyncFrameWriterTest.java

@@ -0,0 +1,151 @@
+/*
+ * Copyright 2015 The gRPC Authors


ejona86 · 2018-08-15T20:07:17Z

okhttp/src/test/java/io/grpc/okhttp/AsyncFrameWriterTest.java

+
+    verify(frameWriter, times(2)).ping(anyBoolean(), anyInt(), anyInt());
+    verify(frameWriter, times(1)).flush();
+    assertThat(Iterables.getLast(PartiallyTrackingFrameWriter.getAllInvokedMethodsHistory()))


Why not use inOrder?

didn't know that exists. thanks!

carl-mastrangelo

One nit but LGTM

carl-mastrangelo · 2018-08-15T21:56:44Z

okhttp/src/test/java/io/grpc/okhttp/AsyncFrameWriterTest.java

+  }
+
+  /**
+   * Executor queues incoming runnables instead of running it. Runnables can be invoked via {@code


this should be #link

ejona86 · 2018-08-16T15:18:09Z

okhttp/src/test/java/io/grpc/okhttp/AsyncFrameWriterTest.java

+
+  @Before
+  public void setUp() throws Exception {
+    queueingExecutor = new QueueingExecutor();


nit: Initialize at the declaration above.

ejona86 · 2018-08-16T15:22:20Z

okhttp/src/test/java/io/grpc/okhttp/AsyncFrameWriterTest.java

+
+  @After
+  public void tearDown() throws Exception {
+    asyncFrameWriter.close();


What's the point of this? It is guaranteed to do nothing. If we wanted to close the (mock) socket there would need to be a runAll() here.

ejona86 · 2018-08-16T15:24:17Z

okhttp/src/test/java/io/grpc/okhttp/AsyncFrameWriterTest.java

+@RunWith(MockitoJUnitRunner.class)
+public class AsyncFrameWriterTest {
+
+  @Mock private OkHttpClientTransport transport;


Mocking a functional class like the transport is worrisome to me. Only a single method is used, could we make an interface for it and have the transport implement that interface?

yes, created an interface TransportExceptionHandler, can you review this part?

…ndler which makes AsyncFrameWriter accept the interface rather than a transport.

ejona86 · 2018-08-17T15:15:48Z

okhttp/src/test/java/io/grpc/okhttp/AsyncFrameWriterTest.java

+
+  @After
+  public void tearDown() throws Exception {
+    queueingExecutor.clear();


This does nothing, as this test class instance will be thrown away. So you're clearing something before it is cleared by the GC.

ejona86 · 2018-08-17T15:16:44Z

okhttp/src/test/java/io/grpc/okhttp/AsyncFrameWriterTest.java

+
+    @Override
+    public void onException(Throwable throwable) {
+      throw Status.INTERNAL.asRuntimeException();


This throws away the Throwable, and we don't use StatusRuntimeException too much like this. How about just new AssertionError(throwable)?

ejona86 · 2018-08-17T15:17:13Z

okhttp/src/test/java/io/grpc/okhttp/AsyncFrameWriterTest.java

+  @Before
+  public void setUp() throws Exception {
+    asyncFrameWriter =
+        new AsyncFrameWriter(transportExceptionHandler, new SerializingExecutor(queueingExecutor));


nit: this could also be moved up to the declaration, now that the transport isn't a mock.

ejona86

Looks good. Make sure to squash when merging.

ejona86 reviewed Aug 15, 2018

View reviewed changes

Implement flush coalescing in OkHttp.

7df7e80

Implementation is skipping flush task if another flush is queued.

creamsoup force-pushed the nameresolver branch from b9a47eb to 7df7e80 Compare August 15, 2018 18:51

remove unnecessary verify

fd2cda8

carl-mastrangelo reviewed Aug 15, 2018

View reviewed changes

creamsoup added 3 commits August 15, 2018 11:57

fix copywrite year

857c478

change the message name to be clear that it returns called method.

5e181b8

remove spy, add teardown for the partial mock

8a8a4a5

ejona86 reviewed Aug 15, 2018

View reviewed changes

using mockito inorder

26ef499

creamsoup force-pushed the nameresolver branch from e1b8e8b to 26ef499 Compare August 15, 2018 21:36

carl-mastrangelo approved these changes Aug 15, 2018

View reviewed changes

fix javadoc

42590dc

ejona86 approved these changes Aug 16, 2018

View reviewed changes

creamsoup added 4 commits August 16, 2018 14:24

Make OkHttpClientTransport extends new interface TransportExceptionHa…

db4a260

…ndler which makes AsyncFrameWriter accept the interface rather than a transport.

move TransportExceptionHandler to inner class, add other changes

0b90430

Move TransportExceptionHandler to inner class of AsyncFrameWriter.

55edfe2

Make TransportExceptionHandler inner interface

ee35e4d

ejona86 reviewed Aug 17, 2018

View reviewed changes

creamsoup added 3 commits August 17, 2018 10:19

clean up code by addressing comments

fc235bf

fix javadoc

9bf5900

remove unused method

574db49

ejona86 approved these changes Aug 17, 2018

View reviewed changes

creamsoup merged commit 30a4bfb into grpc:master Aug 17, 2018

lock bot locked as resolved and limited conversation to collaborators Nov 15, 2018

creamsoup deleted the nameresolver branch May 2, 2019 17:31


		private static abstract class PartiallyTrackingFrameWriter implements FrameWriter {

		private static final List<Method> methods = new ArrayList<Method>();

Implement flush coalescing in OkHttp. #4763

Implement flush coalescing in OkHttp. #4763

Uh oh!

Conversation

creamsoup commented Aug 15, 2018

Uh oh!

ejona86 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carl-mastrangelo 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!

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!

ejona86 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants