----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60451/#review179071 -----------------------------------------------------------
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufOpsProcessor.java Lines 68 (patched) <https://reviews.apache.org/r/60451/#comment253474> This has nothing to do with the ProtobufOpsProcessor. Maybe the builder can be passed in. geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetRequestOperationHandler.java Line 42 (original), 39 (patched) <https://reviews.apache.org/r/60451/#comment253476> Maybe we do this before the key is decoded. After line 34 if possible. geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetRequestOperationHandler.java Lines 42 (patched) <https://reviews.apache.org/r/60451/#comment253475> Maybe this should be replaced with an errorMessage, stating this region does not exist. geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetRequestOperationHandler.java Lines 54 (patched) <https://reviews.apache.org/r/60451/#comment253477> `encoding` should start with capitol geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetRequestOperationHandler.java Lines 56 (patched) <https://reviews.apache.org/r/60451/#comment253478> `codec` should start with capitol geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetRequestOperationHandler.java Lines 62 (patched) <https://reviews.apache.org/r/60451/#comment253470> Maybe this should return an empty list. geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetRequestOperationHandler.java Lines 66 (patched) <https://reviews.apache.org/r/60451/#comment253479> This provides no information about the failure... An `ErrorMessage` should be a more suitable replacement geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetRequestOperationHandler.java Line 59 (original), 78 (patched) <https://reviews.apache.org/r/60451/#comment253480> Can I have a `success=false` and `keyExists=true`? What does `keyExists` provide me in functionality above and beyond `success` or an `ErrorMessage`? geode-protobuf/src/main/proto/region_API.proto Line 35 (original), 35 (patched) <https://reviews.apache.org/r/60451/#comment253471> not sure `success` or `keyExists` provides any value here.... `result` should either have a value or not. What does `keyExists` denote? What counts as a `failure`? geode-protobuf/src/test/java/org/apache/geode/protocol/MessageUtil.java Line 20 (original), 31 (patched) <https://reviews.apache.org/r/60451/#comment253473> Given this class is Protobuf specific, we either put it in the `protobuf` specific package or call it `ProtobufMessageUtil` geode-protobuf/src/test/java/org/apache/geode/protocol/MessageUtil.java Lines 46 (patched) <https://reviews.apache.org/r/60451/#comment253481> This is too specific... What about all the other encoding types? Int,long,short,byte? - Udo Kohlmeyer On June 27, 2017, 1:20 a.m., Brian Rowe wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60451/ > ----------------------------------------------------------- > > (Updated June 27, 2017, 1:20 a.m.) > > > Review request for geode, Alexander Murmann, Bruce Schuchardt, Galen > O'Sullivan, Hitesh Khamesra, and Udo Kohlmeyer. > > > Bugs: GEODE-2996 > https://issues.apache.org/jira/browse/GEODE-2996 > > > Repository: geode > > > Description > ------- > > This is a continuation of the review Alex submitted this morning with the > following changes: > > Addresses review feedback for GEODE-2996, mainly refactoring > getOpertionHandler to handle failures like the putOperationHandler > Adding put operations to the RoundTripCacheConnectionJUnitTest, which is the > integration test for the protobuf module > Removing service loading for protobuf operations and instead have the > ProtobufStreamProcessor populate its OperationHandlerRegistry > Remove exception throwing from OperationHandler.process calls and remove > TypeEncodingException > Fixing ProtobufOpsProcessor to handle responses for types other than get > > > Diffs > ----- > > > geode-protobuf/src/main/java/org/apache/geode/protocol/operations/OperationHandler.java > 7683e3bf3 > > geode-protobuf/src/main/java/org/apache/geode/protocol/operations/registry/OperationsHandlerRegistry.java > 8e3a33149 > > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufOpsProcessor.java > d426149e4 > > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufStreamProcessor.java > d7b5d4bd2 > > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufUtilities.java > d76366298 > > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetRequestOperationHandler.java > d9c14752f > > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/PutRequestOperationHandler.java > PRE-CREATION > > geode-protobuf/src/main/java/org/apache/geode/serialization/exception/TypeEncodingException.java > f3145a774 > > geode-protobuf/src/main/java/org/apache/geode/serialization/exception/UnsupportedEncodingTypeException.java > c577e768a > > geode-protobuf/src/main/java/org/apache/geode/serialization/registry/exception/CodecNotRegisteredForTypeException.java > 5c923a520 > geode-protobuf/src/main/proto/region_API.proto 52291c451 > geode-protobuf/src/test/java/org/apache/geode/protocol/MessageUtil.java > f0b0b417b > > geode-protobuf/src/test/java/org/apache/geode/protocol/RoundTripCacheConnectionJUnitTest.java > b9faca3c9 > > geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/ProtobufOpsProcessorJUnitTest.java > fc980aec9 > > geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/GetRequestOperationHandlerJUnitTest.java > daa5870ed > > geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/PutRequestOperationHandlerJUnitTest.java > PRE-CREATION > > > Diff: https://reviews.apache.org/r/60451/diff/1/ > > > Testing > ------- > > Unit tests, whole module test, precheckin in progress. > > > Thanks, > > Brian Rowe > >
