> On June 28, 2017, 4:24 a.m., Udo Kohlmeyer wrote: > > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufOpsProcessor.java > > Lines 68 (patched) > > <https://reviews.apache.org/r/60451/diff/1/?file=1763372#file1763372line70> > > > > This has nothing to do with the ProtobufOpsProcessor. Maybe the builder > > can be passed in. > > Brian Rowe wrote: > Hmm, I'm not sure I agree. The ProtobufOpsProcessor has to generate the > ClientProtocol.Response containing the response from the operation handler. > It seems reasonable for it to know how to build this object. Having the > builder needing to be passed into this seems similar to requiring this class > to pass the builder for the operation response down to the operation handler.
I would try and contain all the builder code and Protobuf message building stuff to the utility. This way we have a single place where we do anything with the protobuf messages > On June 28, 2017, 4:24 a.m., Udo Kohlmeyer wrote: > > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetRequestOperationHandler.java > > Lines 66 (patched) > > <https://reviews.apache.org/r/60451/diff/1/?file=1763375#file1763375line70> > > > > This provides no information about the failure... An `ErrorMessage` > > should be a more suitable replacement > > Brian Rowe wrote: > Yes, once we have an error message we'll hopefully not even return a > GetResponse here (which will make things more complicated in the > OpsProcessor). I think it would be simple to have a "success" and a "failure" branch in the code.. If there is a failure, we consistently create an `ErrorMessage`. Don't think that logic would be too hard to create or even make it generic for all operations. > On June 28, 2017, 4:24 a.m., Udo Kohlmeyer wrote: > > geode-protobuf/src/test/java/org/apache/geode/protocol/MessageUtil.java > > Line 20 (original), 31 (patched) > > <https://reviews.apache.org/r/60451/diff/1/?file=1763381#file1763381line31> > > > > Given this class is Protobuf specific, we either put it in the > > `protobuf` specific package or call it `ProtobufMessageUtil` > > Brian Rowe wrote: > I like ProtobufTestMessageUtil just to make it clear that it's both > protobuf and test specific. I think we should have 1 util that gets used for both testing and main code base. At least we then make sure that message are always constructed in the same way. I don't like the Protobuf builder code littering the `Handler` implementations. > On June 28, 2017, 4:24 a.m., Udo Kohlmeyer wrote: > > geode-protobuf/src/test/java/org/apache/geode/protocol/MessageUtil.java > > Lines 46 (patched) > > <https://reviews.apache.org/r/60451/diff/1/?file=1763381#file1763381line46> > > > > This is too specific... What about all the other encoding types? > > Int,long,short,byte? > > Brian Rowe wrote: > We discussed templatizing this to make it more generally useful, but > decided that since this is a test utility it wasn't really worth making that > change unless it was needed. As per comment about making a `ProtobugMessageUtil`. This way we have a single class that is responsible for the construction of the Protobuf messages - Udo ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60451/#review179071 ----------------------------------------------------------- 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 > >
