[GitHub] geode pull request #646: GEODE-3213: Refactor ProtoBuf handler flow

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/geode/pull/646


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #646: GEODE-3213: Refactor ProtoBuf handler flow

[kohlmu-pivotal]

Github user kohlmu-pivotal commented on a diff in the pull request:

https://github.com/apache/geode/pull/646#discussion_r128814031
  
--- Diff: 
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/OperationContext.java
 ---
@@ -0,0 +1,57 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more 
contributor license
+ * agreements. See the NOTICE file distributed with this work for 
additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache 
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the 
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software 
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF 
ANY KIND, either express
+ * or implied. See the License for the specific language governing 
permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.protocol.protobuf;
+
+import org.apache.geode.protocol.operations.OperationHandler;
+
+import java.util.function.Function;
+
+public class OperationContext {
--- End diff --

+1


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #646: GEODE-3213: Refactor ProtoBuf handler flow

[galen-pivotal]

Github user galen-pivotal commented on a diff in the pull request:

https://github.com/apache/geode/pull/646#discussion_r128811946
  
--- Diff: 
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/OperationContext.java
 ---
@@ -0,0 +1,57 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more 
contributor license
+ * agreements. See the NOTICE file distributed with this work for 
additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache 
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the 
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software 
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF 
ANY KIND, either express
+ * or implied. See the License for the specific language governing 
permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.protocol.protobuf;
+
+import org.apache.geode.protocol.operations.OperationHandler;
+
+import java.util.function.Function;
+
+public class OperationContext {
--- End diff --

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #646: GEODE-3213: Refactor ProtoBuf handler flow

[galen-pivotal]

Github user galen-pivotal commented on a diff in the pull request:

https://github.com/apache/geode/pull/646#discussion_r128811551
  
--- Diff: 
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufResponseUtilities.java
 ---
@@ -35,10 +37,9 @@
* @param errorMessage - description of the error
* @return An error response containing the above parameters
*/
-  public static ClientProtocol.Response createErrorResponse(String 
errorMessage) {
-ClientProtocol.ErrorResponse error =
-
ClientProtocol.ErrorResponse.newBuilder().setMessage(errorMessage).build();
-return 
ClientProtocol.Response.newBuilder().setErrorResponse(error).build();
+  public static Failure createFailureResult(String errorMessage) {
--- End diff --

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #646: GEODE-3213: Refactor ProtoBuf handler flow

[pivotal-amurmann]

Github user pivotal-amurmann commented on a diff in the pull request:

https://github.com/apache/geode/pull/646#discussion_r128793895
  
--- Diff: 
geode-protobuf/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,
--- End diff --

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #646: GEODE-3213: Refactor ProtoBuf handler flow

[galen-pivotal]

Github user galen-pivotal commented on a diff in the pull request:

https://github.com/apache/geode/pull/646#discussion_r128678978
  
--- Diff: 
geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/PutRequestOperationHandlerJUnitTest.java
 ---
@@ -124,20 +128,19 @@ public void test_RegionThrowsClasscastException() 
throws CodecAlreadyRegisteredF
 when(regionMock.put(any(), any())).thenThrow(ClassCastException.class);
 
 PutRequestOperationHandler operationHandler = new 
PutRequestOperationHandler();
-ClientProtocol.Response response =
+Result result =
 operationHandler.process(serializationServiceStub, 
generateTestRequest(), cacheStub);
 
-
Assert.assertEquals(ClientProtocol.Response.ResponseAPICase.ERRORRESPONSE,
-response.getResponseAPICase());
+Assert.assertTrue(result instanceof Failure);
--- End diff --

no. Is it worth asserting part of it?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #646: GEODE-3213: Refactor ProtoBuf handler flow

[galen-pivotal]

Github user galen-pivotal commented on a diff in the pull request:

https://github.com/apache/geode/pull/646#discussion_r128678905
  
--- Diff: 
geode-protobuf/src/test/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
--- End diff --

👍  getting rid of typecasts.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #646: GEODE-3213: Refactor ProtoBuf handler flow

[galen-pivotal]

Github user galen-pivotal commented on a diff in the pull request:

https://github.com/apache/geode/pull/646#discussion_r128677601
  
--- Diff: 
geode-protobuf/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,
--- End diff --

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #646: GEODE-3213: Refactor ProtoBuf handler flow

[galen-pivotal]

Github user galen-pivotal commented on a diff in the pull request:

https://github.com/apache/geode/pull/646#discussion_r128649982
  
--- Diff: 
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/OperationContext.java
 ---
@@ -0,0 +1,57 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more 
contributor license
+ * agreements. See the NOTICE file distributed with this work for 
additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache 
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the 
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software 
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF 
ANY KIND, either express
+ * or implied. See the License for the specific language governing 
permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.protocol.protobuf;
+
+import org.apache.geode.protocol.operations.OperationHandler;
+
+import java.util.function.Function;
+
+public class OperationContext {
+  private OperationHandler 
operationHandler;
--- End diff --

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.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #646: GEODE-3213: Refactor ProtoBuf handler flow

[galen-pivotal]

Github user galen-pivotal commented on a diff in the pull request:

https://github.com/apache/geode/pull/646#discussion_r128649664
  
--- Diff: 
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/OperationContext.java
 ---
@@ -0,0 +1,57 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more 
contributor license
+ * agreements. See the NOTICE file distributed with this work for 
additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache 
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the 
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software 
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF 
ANY KIND, either express
+ * or implied. See the License for the specific language governing 
permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.protocol.protobuf;
+
+import org.apache.geode.protocol.operations.OperationHandler;
+
+import java.util.function.Function;
+
+public class OperationContext {
+  private OperationHandler 
operationHandler;
--- End diff --

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.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #646: GEODE-3213: Refactor ProtoBuf handler flow

[galen-pivotal]

Github user galen-pivotal commented on a diff in the pull request:

https://github.com/apache/geode/pull/646#discussion_r128649345
  
--- Diff: 
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/OperationContext.java
 ---
@@ -0,0 +1,57 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more 
contributor license
+ * agreements. See the NOTICE file distributed with this work for 
additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache 
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the 
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software 
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF 
ANY KIND, either express
+ * or implied. See the License for the specific language governing 
permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.protocol.protobuf;
+
+import org.apache.geode.protocol.operations.OperationHandler;
+
+import java.util.function.Function;
+
+public class OperationContext {
+  private OperationHandler 
operationHandler;
--- End diff --

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.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #646: GEODE-3213: Refactor ProtoBuf handler flow

[galen-pivotal]

Github user galen-pivotal commented on a diff in the pull request:

https://github.com/apache/geode/pull/646#discussion_r128647952
  
--- Diff: 
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/OperationContext.java
 ---
@@ -0,0 +1,57 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more 
contributor license
+ * agreements. See the NOTICE file distributed with this work for 
additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache 
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the 
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software 
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF 
ANY KIND, either express
+ * or implied. See the License for the specific language governing 
permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.protocol.protobuf;
+
+import org.apache.geode.protocol.operations.OperationHandler;
+
+import java.util.function.Function;
+
+public class OperationContext {
+  private OperationHandler 
operationHandler;
--- End diff --

I think these fields should be final.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #646: GEODE-3213: Refactor ProtoBuf handler flow

[galen-pivotal]

Github user galen-pivotal commented on a diff in the pull request:

https://github.com/apache/geode/pull/646#discussion_r128648216
  
--- Diff: 
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufOpsProcessor.java
 ---
@@ -15,32 +15,33 @@
 package org.apache.geode.protocol.protobuf;
 
 import org.apache.geode.cache.Cache;
-import org.apache.geode.protocol.exception.InvalidProtocolMessageException;
-import org.apache.geode.protocol.operations.OperationHandler;
-import 
org.apache.geode.protocol.operations.registry.OperationsHandlerRegistry;
-import 
org.apache.geode.protocol.operations.registry.exception.OperationHandlerNotRegisteredException;
+import 
org.apache.geode.protocol.protobuf.registry.OperationContextRegistry;
 import org.apache.geode.serialization.SerializationService;
 
 /**
  * This handles protobuf requests by determining the operation type of the 
request and dispatching
  * it to the appropriate handler.
  */
 public class ProtobufOpsProcessor {
-  private final OperationsHandlerRegistry opsHandlerRegistry;
+
+  private final OperationContextRegistry operationContextRegistry;
   private final SerializationService serializationService;
 
-  public ProtobufOpsProcessor(OperationsHandlerRegistry opsHandlerRegistry,
-  SerializationService serializationService) {
-this.opsHandlerRegistry = opsHandlerRegistry;
+  public ProtobufOpsProcessor(SerializationService serializationService,
+  OperationContextRegistry operationsContextRegistry) {
--- End diff --

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #646: GEODE-3213: Refactor ProtoBuf handler flow

[galen-pivotal]

Github user galen-pivotal commented on a diff in the pull request:

https://github.com/apache/geode/pull/646#discussion_r128649165
  
--- Diff: 
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/OperationContext.java
 ---
@@ -0,0 +1,57 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more 
contributor license
+ * agreements. See the NOTICE file distributed with this work for 
additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache 
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the 
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software 
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF 
ANY KIND, either express
+ * or implied. See the License for the specific language governing 
permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.protocol.protobuf;
+
+import org.apache.geode.protocol.operations.OperationHandler;
+
+import java.util.function.Function;
+
+public class OperationContext {
+  private OperationHandler 
operationHandler;
+  private Function fromRequest;
+  private Function 
toResponse;
+  private Function toErrorResponse;
+
+  public OperationContext(Function 
fromRequest,
--- End diff --

Can we use `OperationRequest` and `OperationResponse`?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #646: GEODE-3213: Refactor ProtoBuf handler flow

[galen-pivotal]

Github user galen-pivotal commented on a diff in the pull request:

https://github.com/apache/geode/pull/646#discussion_r128649824
  
--- Diff: 
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/OperationContext.java
 ---
@@ -0,0 +1,57 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more 
contributor license
+ * agreements. See the NOTICE file distributed with this work for 
additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache 
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the 
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software 
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF 
ANY KIND, either express
+ * or implied. See the License for the specific language governing 
permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.protocol.protobuf;
+
+import org.apache.geode.protocol.operations.OperationHandler;
+
+import java.util.function.Function;
+
+public class OperationContext {
+  private OperationHandler 
operationHandler;
--- End diff --

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #646: GEODE-3213: Refactor ProtoBuf handler flow

[pivotal-amurmann]

Github user pivotal-amurmann commented on a diff in the pull request:

https://github.com/apache/geode/pull/646#discussion_r128648851
  
--- Diff: 
geode-protobuf/src/main/java/org/apache/geode/protocol/operations/Failure.java 
---
@@ -0,0 +1,47 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more 
contributor license
+ * agreements. See the NOTICE file distributed with this work for 
additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache 
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the 
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software 
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF 
ANY KIND, either express
+ * or implied. See the License for the specific language governing 
permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.protocol.operations;
--- End diff --

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. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #646: GEODE-3213: Refactor ProtoBuf handler flow

[pivotal-amurmann]

Github user pivotal-amurmann commented on a diff in the pull request:

https://github.com/apache/geode/pull/646#discussion_r128618059
  
--- Diff: 
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufOpsProcessor.java
 ---
@@ -15,32 +15,35 @@
 package org.apache.geode.protocol.protobuf;
 
 import org.apache.geode.cache.Cache;
-import org.apache.geode.protocol.exception.InvalidProtocolMessageException;
-import org.apache.geode.protocol.operations.OperationHandler;
-import 
org.apache.geode.protocol.operations.registry.OperationsHandlerRegistry;
-import 
org.apache.geode.protocol.operations.registry.exception.OperationHandlerNotRegisteredException;
+import org.apache.geode.protocol.operations.OperationContext;
+import org.apache.geode.protocol.operations.Result;
+import 
org.apache.geode.protocol.operations.registry.OperationContextRegistry;
 import org.apache.geode.serialization.SerializationService;
 
 /**
  * This handles protobuf requests by determining the operation type of the 
request and dispatching
  * it to the appropriate handler.
  */
 public class ProtobufOpsProcessor {
-  private final OperationsHandlerRegistry opsHandlerRegistry;
+
+  private final OperationContextRegistry operationContextRegistry;
   private final SerializationService serializationService;
 
-  public ProtobufOpsProcessor(OperationsHandlerRegistry opsHandlerRegistry,
-  SerializationService serializationService) {
-this.opsHandlerRegistry = opsHandlerRegistry;
+  public ProtobufOpsProcessor(SerializationService serializationService,
+  OperationContextRegistry operationsContextRegistry) {
 this.serializationService = serializationService;
+this.operationContextRegistry = operationsContextRegistry;
   }
 
-  public ClientProtocol.Response process(ClientProtocol.Request request, 
Cache cache)
-  throws OperationHandlerNotRegisteredException, 
InvalidProtocolMessageException {
+  public ClientProtocol.Response process(ClientProtocol.Request request, 
Cache cache) {
 ClientProtocol.Request.RequestAPICase requestType = 
request.getRequestAPICase();
-OperationHandler opsHandler =
-
opsHandlerRegistry.getOperationHandlerForOperationId(requestType.getNumber());
+OperationContext operationContext = 
operationContextRegistry.getOperationContext(requestType);
--- End diff --

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.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #646: GEODE-3213: Refactor ProtoBuf handler flow

[WireBaron]

Github user WireBaron commented on a diff in the pull request:

https://github.com/apache/geode/pull/646#discussion_r128591891
  
--- Diff: 
geode-protobuf/src/main/java/org/apache/geode/protocol/operations/Result.java 
---
@@ -0,0 +1,28 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more 
contributor license
+ * agreements. See the NOTICE file distributed with this work for 
additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache 
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the 
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software 
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF 
ANY KIND, either express
+ * or implied. See the License for the specific language governing 
permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.protocol.operations;
+
+import org.apache.geode.protocol.protobuf.ClientProtocol;
+
+import java.util.function.Function;
+
+public interface Result {
--- End diff --

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #646: GEODE-3213: Refactor ProtoBuf handler flow

[WireBaron]

Github user WireBaron commented on a diff in the pull request:

https://github.com/apache/geode/pull/646#discussion_r128597628
  
--- Diff: 
geode-protobuf/src/main/java/org/apache/geode/protocol/operations/Failure.java 
---
@@ -0,0 +1,47 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more 
contributor license
+ * agreements. See the NOTICE file distributed with this work for 
additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache 
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the 
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software 
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF 
ANY KIND, either express
+ * or implied. See the License for the specific language governing 
permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.protocol.operations;
--- End diff --

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?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #646: GEODE-3213: Refactor ProtoBuf handler flow

[WireBaron]

Github user WireBaron commented on a diff in the pull request:

https://github.com/apache/geode/pull/646#discussion_r128600182
  
--- Diff: 
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufOpsProcessor.java
 ---
@@ -15,32 +15,35 @@
 package org.apache.geode.protocol.protobuf;
 
 import org.apache.geode.cache.Cache;
-import org.apache.geode.protocol.exception.InvalidProtocolMessageException;
-import org.apache.geode.protocol.operations.OperationHandler;
-import 
org.apache.geode.protocol.operations.registry.OperationsHandlerRegistry;
-import 
org.apache.geode.protocol.operations.registry.exception.OperationHandlerNotRegisteredException;
+import org.apache.geode.protocol.operations.OperationContext;
+import org.apache.geode.protocol.operations.Result;
+import 
org.apache.geode.protocol.operations.registry.OperationContextRegistry;
 import org.apache.geode.serialization.SerializationService;
 
 /**
  * This handles protobuf requests by determining the operation type of the 
request and dispatching
  * it to the appropriate handler.
  */
 public class ProtobufOpsProcessor {
-  private final OperationsHandlerRegistry opsHandlerRegistry;
+
+  private final OperationContextRegistry operationContextRegistry;
   private final SerializationService serializationService;
 
-  public ProtobufOpsProcessor(OperationsHandlerRegistry opsHandlerRegistry,
-  SerializationService serializationService) {
-this.opsHandlerRegistry = opsHandlerRegistry;
+  public ProtobufOpsProcessor(SerializationService serializationService,
+  OperationContextRegistry operationsContextRegistry) {
 this.serializationService = serializationService;
+this.operationContextRegistry = operationsContextRegistry;
   }
 
-  public ClientProtocol.Response process(ClientProtocol.Request request, 
Cache cache)
-  throws OperationHandlerNotRegisteredException, 
InvalidProtocolMessageException {
+  public ClientProtocol.Response process(ClientProtocol.Request request, 
Cache cache) {
 ClientProtocol.Request.RequestAPICase requestType = 
request.getRequestAPICase();
-OperationHandler opsHandler =
-
opsHandlerRegistry.getOperationHandlerForOperationId(requestType.getNumber());
+OperationContext operationContext = 
operationContextRegistry.getOperationContext(requestType);
--- End diff --

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #646: GEODE-3213: Refactor ProtoBuf handler flow

[kohlmu-pivotal]

Github user kohlmu-pivotal commented on a diff in the pull request:

https://github.com/apache/geode/pull/646#discussion_r128567505
  
--- Diff: 
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufResponseUtilities.java
 ---
@@ -67,30 +68,27 @@
*{@link ProtobufUtilities}
* @return A response indicating the passed value was found for a 
incoming GetRequest
*/
-  public static ClientProtocol.Response 
createGetResponse(BasicTypes.EncodedValue resultValue) {
-RegionAPI.GetResponse getResponse =
-RegionAPI.GetResponse.newBuilder().setResult(resultValue).build();
-return 
ClientProtocol.Response.newBuilder().setGetResponse(getResponse).build();
+  public static Success createGetResult(
+  BasicTypes.EncodedValue resultValue) {
+return new 
Success<>(RegionAPI.GetResponse.newBuilder().setResult(resultValue).build());
--- End diff --

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #646: GEODE-3213: Refactor ProtoBuf handler flow

[kohlmu-pivotal]

Github user kohlmu-pivotal commented on a diff in the pull request:

https://github.com/apache/geode/pull/646#discussion_r128386349
  
--- Diff: 
geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/PutRequestOperationHandlerJUnitTest.java
 ---
@@ -124,20 +128,19 @@ public void test_RegionThrowsClasscastException() 
throws CodecAlreadyRegisteredF
 when(regionMock.put(any(), any())).thenThrow(ClassCastException.class);
 
 PutRequestOperationHandler operationHandler = new 
PutRequestOperationHandler();
-ClientProtocol.Response response =
+Result result =
 operationHandler.process(serializationServiceStub, 
generateTestRequest(), cacheStub);
 
-
Assert.assertEquals(ClientProtocol.Response.ResponseAPICase.ERRORRESPONSE,
-response.getResponseAPICase());
+Assert.assertTrue(result instanceof Failure);
--- End diff --

Do we need to assert at the error message


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #646: GEODE-3213: Refactor ProtoBuf handler flow

[kohlmu-pivotal]

Github user kohlmu-pivotal commented on a diff in the pull request:

https://github.com/apache/geode/pull/646#discussion_r128386432
  
--- Diff: 
geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/PutRequestOperationHandlerJUnitTest.java
 ---
@@ -99,23 +105,21 @@ public void test_codecNotRegistered() throws 
CodecAlreadyRegisteredForTypeExcept
 TEST_KEY.getBytes(Charset.forName("UTF-8".thenThrow(exception);
 PutRequestOperationHandler operationHandler = new 
PutRequestOperationHandler();
 
-ClientProtocol.Response response =
+Result result =
 operationHandler.process(serializationServiceStub, 
generateTestRequest(), cacheStub);
 
-
Assert.assertEquals(ClientProtocol.Response.ResponseAPICase.ERRORRESPONSE,
-response.getResponseAPICase());
+Assert.assertTrue(result instanceof Failure);
   }
 
   @Test
   public void test_RegionNotFound() throws 
CodecAlreadyRegisteredForTypeException,
   UnsupportedEncodingTypeException, CodecNotRegisteredForTypeException 
{
 when(cacheStub.getRegion(TEST_REGION)).thenReturn(null);
 PutRequestOperationHandler operationHandler = new 
PutRequestOperationHandler();
-ClientProtocol.Response response =
+Result result =
 operationHandler.process(serializationServiceStub, 
generateTestRequest(), cacheStub);
 
-
Assert.assertEquals(ClientProtocol.Response.ResponseAPICase.ERRORRESPONSE,
-response.getResponseAPICase());
+Assert.assertTrue(result instanceof Failure);
--- End diff --

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #646: GEODE-3213: Refactor ProtoBuf handler flow

[kohlmu-pivotal]

Github user kohlmu-pivotal commented on a diff in the pull request:

https://github.com/apache/geode/pull/646#discussion_r128388403
  
--- Diff: 
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufResponseUtilities.java
 ---
@@ -35,10 +37,9 @@
* @param errorMessage - description of the error
* @return An error response containing the above parameters
*/
-  public static ClientProtocol.Response createErrorResponse(String 
errorMessage) {
-ClientProtocol.ErrorResponse error =
-
ClientProtocol.ErrorResponse.newBuilder().setMessage(errorMessage).build();
-return 
ClientProtocol.Response.newBuilder().setErrorResponse(error).build();
+  public static Failure createFailureResult(String errorMessage) {
--- End diff --

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #646: GEODE-3213: Refactor ProtoBuf handler flow

[kohlmu-pivotal]

Github user kohlmu-pivotal commented on a diff in the pull request:

https://github.com/apache/geode/pull/646#discussion_r128387458
  
--- Diff: 
geode-protobuf/src/main/java/org/apache/geode/protocol/operations/OperationContext.java
 ---
@@ -0,0 +1,57 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more 
contributor license
+ * agreements. See the NOTICE file distributed with this work for 
additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache 
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the 
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software 
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF 
ANY KIND, either express
+ * or implied. See the License for the specific language governing 
permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.protocol.operations;
+
+import org.apache.geode.protocol.protobuf.ClientProtocol;
+
+import java.util.function.Function;
+
+public class OperationContext {
--- End diff --

Would it make sense to maybe package restrict this class


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #646: GEODE-3213: Refactor ProtoBuf handler flow

[kohlmu-pivotal]

Github user kohlmu-pivotal commented on a diff in the pull request:

https://github.com/apache/geode/pull/646#discussion_r128386591
  
--- Diff: 
geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/PutRequestOperationHandlerJUnitTest.java
 ---
@@ -64,11 +72,10 @@ public void setUp() throws Exception {
   public void test_puttingTheEncodedEntryIntoRegion() throws 
UnsupportedEncodingTypeException,
   CodecNotRegisteredForTypeException, 
CodecAlreadyRegisteredForTypeException {
 PutRequestOperationHandler operationHandler = new 
PutRequestOperationHandler();
-ClientProtocol.Response response =
+Result result =
 operationHandler.process(serializationServiceStub, 
generateTestRequest(), cacheStub);
 
-
Assert.assertEquals(ClientProtocol.Response.ResponseAPICase.PUTRESPONSE,
-response.getResponseAPICase());
+Assert.assertNotNull(result);
--- End diff --

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---