You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Brian Rowe <br...@pivotal.io> on 2017/07/07 19:53:37 UTC
Re: Review Request 60451: GEODE-2996: adding Put handler
> 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
>
>