> 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. > > Udo Kohlmeyer wrote: > 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
In the change for GEODE-3129 all of the builder specific code got moved into utility objects. > 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. > > Brian Rowe wrote: > I'm going to talk with the team tomorrow and see if we can't come up with > some way to indicate optional arguments. Will return an empty EncodedValue in 3129 change. > 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). > > Udo Kohlmeyer wrote: > 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. Success is removed in 3129. > 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`? > > Brian Rowe wrote: > 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. Removed in 3129 change. > 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`? > > Brian Rowe wrote: > 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. Added proper errorResponse in 3129. > 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. > > Udo Kohlmeyer wrote: > 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. In the change for GEDOE-3129, separated this into test specific code which still lives here and request building functions which got moved into src/.../ProtobufRequestUtilities > 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. > > Udo Kohlmeyer wrote: > As per comment about making a `ProtobugMessageUtil`. This way we have a > single class that is responsible for the construction of the Protobuf messages The one class was a bit hard to tell what was included, so created three classes, one focused on requests, one on responses, and one on the other protobuf pieces. - 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 > >
