> 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.
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. > On June 28, 2017, 4:24 a.m., Udo Kohlmeyer wrote: > > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetRequestOperationHandler.java > > Line 42 (original), 39 (patched) > > <https://reviews.apache.org/r/60451/diff/1/?file=1763375#file1763375line42> > > > > Maybe we do this before the key is decoded. After line 34 if possible. Good call. > 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 42 (patched) > > <https://reviews.apache.org/r/60451/diff/1/?file=1763375#file1763375line46> > > > > Maybe this should be replaced with an errorMessage, stating this region > > does not exist. Agreed, we actually have a story to add this in and apparently there's been a discussion with the PDD team about how they want these formatted. All of this error handling will be revisited when that story is picked up (should be in the next few days). > 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 54 (patched) > > <https://reviews.apache.org/r/60451/diff/1/?file=1763375#file1763375line58> > > > > `encoding` should start with capitol Fixed. > 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 56 (patched) > > <https://reviews.apache.org/r/60451/diff/1/?file=1763375#file1763375line60> > > > > `codec` should start with capitol Fixed. > 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 62 (patched) > > <https://reviews.apache.org/r/60451/diff/1/?file=1763375#file1763375line66> > > > > Maybe this should return an empty list. I'm going to talk with the team tomorrow and see if we can't come up with some way to indicate optional arguments. > 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 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). > On June 28, 2017, 4:24 a.m., Udo Kohlmeyer wrote: > > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetRequestOperationHandler.java > > Line 59 (original), 78 (patched) > > <https://reviews.apache.org/r/60451/diff/1/?file=1763375#file1763375line82> > > > > Can I have a `success=false` and `keyExists=true`? What does > > `keyExists` provide me in functionality above and beyond `success` or an > > `ErrorMessage`? keyExists was supposed to differentiate between not present vs. nulled keys. However in talking through this with Galen it's clear that we need to think through this a bit more. > On June 28, 2017, 4:24 a.m., Udo Kohlmeyer wrote: > > geode-protobuf/src/main/proto/region_API.proto > > Line 35 (original), 35 (patched) > > <https://reviews.apache.org/r/60451/diff/1/?file=1763380#file1763380line35> > > > > 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`? Unfortunately the protobuf protocol doesn't really give us a way to not have a result. keyExists is supposed to denote whether the key was found independant of whether or not it had a value, but we still need to figure out how to encode a lack of value. A failure would be anything that kept us from even running the query. An error decoding the key would be an example. > 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` I like ProtobufTestMessageUtil just to make it clear that it's both protobuf and test specific. > 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? 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. - Brian ----------------------------------------------------------- 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 > >
