GEODE-3213: Refactor ProtoBuf handler flow #646

pivotal-amurmann · 2017-07-19T21:49:24Z

Signed-off-by: Alexander Murmann amurmann@pivotal.io

Thank you for submitting a contribution to Apache Geode.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
GEODE-3213
Has your PR been rebased against the latest commit within the target branch (typically develop)?
Is your initial contribution a single, squashed commit?
Does gradlew build run cleanly?
Have you written or updated unit tests to verify your changes?
If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
NA

Note:

Please ensure that once the PR is submitted, you check travis-ci for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.

pivotal-amurmann · 2017-07-19T21:50:33Z

@kohlmu-pivotal @WireBaron @hiteshk25 @galen-pivotal @bschuchardt

kohlmu-pivotal · 2017-07-19T22:39:29Z

.../java/org/apache/geode/protocol/protobuf/operations/PutRequestOperationHandlerJUnitTest.java

        operationHandler.process(serializationServiceStub, generateTestRequest(), cacheStub);

-    Assert.assertEquals(ClientProtocol.Response.ResponseAPICase.ERRORRESPONSE,
-        response.getResponseAPICase());
+    Assert.assertTrue(result instanceof Failure);


Do we need to assert at the error message

no. Is it worth asserting part of it?

kohlmu-pivotal · 2017-07-19T22:40:09Z

.../java/org/apache/geode/protocol/protobuf/operations/PutRequestOperationHandlerJUnitTest.java

        operationHandler.process(serializationServiceStub, generateTestRequest(), cacheStub);

-    Assert.assertEquals(ClientProtocol.Response.ResponseAPICase.ERRORRESPONSE,
-        response.getResponseAPICase());
+    Assert.assertTrue(result instanceof Failure);


We need to assert at least the error message. Maybe later on the error code

kohlmu-pivotal · 2017-07-19T22:41:03Z

.../java/org/apache/geode/protocol/protobuf/operations/PutRequestOperationHandlerJUnitTest.java

        operationHandler.process(serializationServiceStub, generateTestRequest(), cacheStub);

-    Assert.assertEquals(ClientProtocol.Response.ResponseAPICase.PUTRESPONSE,
-        response.getResponseAPICase());
+    Assert.assertNotNull(result);


We should assert that we got back a Success Object rather than just result != null

kohlmu-pivotal · 2017-07-19T22:47:24Z

geode-protobuf/src/main/java/org/apache/geode/protocol/operations/OperationContext.java

+
+import java.util.function.Function;
+
+public class OperationContext<OperationReq, OperationResponse> {


Would it make sense to maybe package restrict this class

kohlmu-pivotal · 2017-07-19T22:53:50Z

...uf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufResponseUtilities.java

-    ClientProtocol.ErrorResponse error =
-        ClientProtocol.ErrorResponse.newBuilder().setMessage(errorMessage).build();
-    return ClientProtocol.Response.newBuilder().setErrorResponse(error).build();
+  public static Failure createFailureResult(String errorMessage) {


I disagree with this method description or its implementation. Either the method name should state createFailureResultWithErrorResponseForErrorMessage OR the signature becomes createFailureResult(ErrorResponse errorResponse).

@pivotal-amurmann @kohlmu-pivotal Did this get addressed?

kohlmu-pivotal · 2017-07-20T16:38:00Z

...uf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufResponseUtilities.java

-    return ClientProtocol.Response.newBuilder().setGetResponse(getResponse).build();
+  public static Success<RegionAPI.GetResponse> createGetResult(
+      BasicTypes.EncodedValue resultValue) {
+    return new Success<>(RegionAPI.GetResponse.newBuilder().setResult(resultValue).build());


could we replace these types of calls with inline Success.of(EncodedValue)

WireBaron

Still reviewing, but here's some initial comments.

WireBaron · 2017-07-20T18:15:44Z

geode-protobuf/src/main/java/org/apache/geode/protocol/operations/Result.java

+
+import java.util.function.Function;
+
+public interface Result<SuccessType> {


This is tightly bound to the ClientProtocol.ErrorResponse, can we give it a less generic name? Maybe ProtocolHandlerResponse?

WireBaron · 2017-07-20T18:37:56Z

geode-protobuf/src/main/java/org/apache/geode/protocol/operations/Failure.java

+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.protocol.operations;


In terms of package layout, I had thought the intent was to have generic code that could be used for any protocol in this package, while protobuf specific code would be under org.apache.geode.protocol.protobuf.* All of the Result and OperationContext code is now protobuf specific. Should it be moved?

We wrote a test that enforces that every enum value has a handler. Of course that test won't pass until we have implemented all operations which isn't going to happen any time soon. So we removed it again. By the time all operations are implemented the test would basically enforce that we don't delete any of the operations and hook them up correctly. However, that's already enforced via the individual integration tests. So it's a very low value test.

WireBaron · 2017-07-20T18:47:42Z

geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufOpsProcessor.java

    ClientProtocol.Request.RequestAPICase requestType = request.getRequestAPICase();
-    OperationHandler opsHandler =
-        opsHandlerRegistry.getOperationHandlerForOperationId(requestType.getNumber());
+    OperationContext operationContext = operationContextRegistry.getOperationContext(requestType);


How are we dealing with missing registry entries now that we're no longer throwing OperationNotRegisteredException?

Currently we are getting a NPE for those which is awful and already lead to wasted time debugging the non-obvious error. Let's go back to throwing something specific.

galen-pivotal

Partial review so far, a few comments.

I'm wondering how much the OperationHandlerContext adds here, or whether it would be cleaner to call through. On the other hand, so far it doesn't look worse than what we have so far.

galen-pivotal · 2017-07-20T22:42:11Z

geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/OperationContext.java

+import java.util.function.Function;
+
+public class OperationContext<OperationReq, OperationResponse> {
+  private OperationHandler<OperationReq, OperationResponse> operationHandler;


I think these fields should be final.

These could be public final and the getters removed. getFromRequest and getToResponse make it sound like the builder is doing something, when all it's doing is returning an imutable final field.

Or we could even write a fromRequest (etc. family of) method that calls fromRequest.apply() on the arguments. Or we could have the context just have a process method that does things in the right order. I'm wondering what's the best way to write this.

We could even just make instances of some interface and not have references to these Functions at all.

I don't think there's a clear best way to do this, feel free to ignore this comment chain and carry on if you want.

galen-pivotal · 2017-07-20T22:43:58Z

geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufOpsProcessor.java

-      SerializationService serializationService) {
-    this.opsHandlerRegistry = opsHandlerRegistry;
+  public ProtobufOpsProcessor(SerializationService serializationService,
+      OperationContextRegistry operationsContextRegistry) {


seems like a nitpick, but why are we using both operation and operations in names here?

galen-pivotal · 2017-07-20T22:49:56Z

geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/OperationContext.java

+  private Function<OperationResponse, ClientProtocol.Response.Builder> toResponse;
+  private Function<ClientProtocol.ErrorResponse, ClientProtocol.Response.Builder> toErrorResponse;
+
+  public OperationContext(Function<ClientProtocol.Request, OperationReq> fromRequest,


Can we use OperationRequest and OperationResponse?

galen-pivotal · 2017-07-21T03:30:19Z

...uf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufResponseUtilities.java

@@ -50,47 +38,14 @@
   * @param ex - exception which should be logged
   * @return An error response containing the first three parameters.
   */
-  public static ClientProtocol.Response createAndLogErrorResponse(String errorMessage,
+  public static ClientProtocol.ErrorResponse createAndLogFailureResponse(String errorMessage,


why'd the name change to FailureResponse when the type changed to ErrorResponse?

I think those were to changes that were made independently. Syncing this up right now

galen-pivotal · 2017-07-21T03:48:25Z

.../java/org/apache/geode/protocol/protobuf/operations/GetRequestOperationHandlerJUnitTest.java

@@ -144,21 +138,22 @@ public void processReturnsErrorWhenUnableToDecodeRequest()
    when(serializationServiceStub.decode(BasicTypes.EncodingType.STRING,
        TEST_KEY.getBytes(Charset.forName("UTF-8")))).thenThrow(exception);

-    ClientProtocol.Request getRequest = generateTestRequest(false, false, false);
-    ClientProtocol.Response response = (ClientProtocol.Response) operationHandler


👍 getting rid of typecasts.

galen-pivotal

I like the Result type. It's interesting trying to do functional-style programming in Java. I think this code is better than what we have now and approve of shipping it.

galen-pivotal · 2017-07-21T17:05:56Z

geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/OperationContext.java

+
+import java.util.function.Function;
+
+public class OperationContext<OperationRequest, OperationResponse> {


I think it makes more sense to have these be implementations of an interface, since they're acting as an immutable collection of functions.

PurelyApplied · 2017-07-21T23:16:35Z

If you are using IntelliJ as your IDE, please remember to update your style file to that located in geode/etc/intellij-java-modified-google-style.xml to be consistent with Geode's established style guide. This file was updated June 13 with commit a561bd12 to be consistent with documented expectations. If you are using Eclipse, please import both style guidefiles, also located in geode/etc/.

After having done so, please optimize the imports to the correct order and to eliminate the use of the wildcard imports from the 25 offending files this pull request touches.

Signed-off-by: Alexander Murmann <amurmann@pivotal.io>

kohlmu-pivotal reviewed Jul 20, 2017

View reviewed changes

pivotal-amurmann force-pushed the GEODE-3213 branch from bc9de53 to b36e62c Compare July 20, 2017 18:28

WireBaron reviewed Jul 20, 2017

View reviewed changes

pivotal-amurmann force-pushed the GEODE-3213 branch from b36e62c to 102114f Compare July 20, 2017 20:05

kohlmu-pivotal approved these changes Jul 20, 2017

View reviewed changes

pivotal-amurmann force-pushed the GEODE-3213 branch from 102114f to ff66707 Compare July 20, 2017 21:15

galen-pivotal reviewed Jul 20, 2017

View reviewed changes

pivotal-amurmann force-pushed the GEODE-3213 branch from ff66707 to 510a42a Compare July 20, 2017 23:30

WireBaron approved these changes Jul 20, 2017

View reviewed changes

galen-pivotal reviewed Jul 21, 2017

View reviewed changes

pivotal-amurmann force-pushed the GEODE-3213 branch from 510a42a to e0ebe4a Compare July 21, 2017 15:41

galen-pivotal approved these changes Jul 21, 2017

View reviewed changes

galen-pivotal reviewed Jul 21, 2017

View reviewed changes

GEODE-3213: Refactor ProtoBuf handler flow

f2896f3

Signed-off-by: Alexander Murmann <amurmann@pivotal.io>

pivotal-amurmann force-pushed the GEODE-3213 branch from e0ebe4a to f2896f3 Compare July 21, 2017 23:55

asfgit closed this in 64eab45 Jul 22, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GEODE-3213: Refactor ProtoBuf handler flow #646

GEODE-3213: Refactor ProtoBuf handler flow #646

pivotal-amurmann commented Jul 19, 2017

pivotal-amurmann commented Jul 19, 2017

kohlmu-pivotal Jul 19, 2017

galen-pivotal Jul 21, 2017

kohlmu-pivotal Jul 19, 2017

kohlmu-pivotal Jul 19, 2017

kohlmu-pivotal Jul 19, 2017

kohlmu-pivotal Jul 19, 2017

galen-pivotal Jul 21, 2017

kohlmu-pivotal Jul 20, 2017

WireBaron left a comment

WireBaron Jul 20, 2017

WireBaron Jul 20, 2017

pivotal-amurmann Jul 20, 2017

WireBaron Jul 20, 2017

pivotal-amurmann Jul 20, 2017

galen-pivotal left a comment

galen-pivotal Jul 20, 2017

galen-pivotal Jul 20, 2017

galen-pivotal Jul 20, 2017

galen-pivotal Jul 20, 2017

galen-pivotal Jul 20, 2017

galen-pivotal Jul 20, 2017

galen-pivotal Jul 20, 2017

galen-pivotal Jul 21, 2017

pivotal-amurmann Jul 21, 2017

galen-pivotal Jul 21, 2017

galen-pivotal left a comment

galen-pivotal Jul 21, 2017

kohlmu-pivotal Jul 21, 2017

PurelyApplied commented Jul 21, 2017


		import java.util.function.Function;

		public class OperationContext<OperationReq, OperationResponse> {


		import java.util.function.Function;

		public interface Result<SuccessType> {

GEODE-3213: Refactor ProtoBuf handler flow #646

GEODE-3213: Refactor ProtoBuf handler flow #646

Conversation

pivotal-amurmann commented Jul 19, 2017

For all changes:

Note:

pivotal-amurmann commented Jul 19, 2017

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

WireBaron 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

galen-pivotal 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

galen-pivotal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PurelyApplied commented Jul 21, 2017