> On July 13, 2017, 1:40 a.m., Galen O'Sullivan wrote: > > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetAllRequestOperationHandler.java > > Lines 59 (patched) > > <https://reviews.apache.org/r/60718/diff/4/?file=1775030#file1775030line59> > > > > It looks like this will throw `ClassCastException` if the key type > > doesn't satisfy key constraints. `LocalRegion`'s implementation calls > > `basicGetAll` to `accessEntry` to `validateKey`. > > > > I also followed `DataSetRegion`'s implementation through the code to > > ``` > > LocalRegion.get(Object key, Object aCallbackArgument, boolean > > generateCallbacks, > > boolean disableCopyOnRead, boolean preferCD, > > ClientProxyMembershipID requestingClient, > > EntryEventImpl clientEvent, boolean returnTombstones, boolean > > opScopeIsLocal, > > boolean retainResult) throws TimeoutException, > > CacheLoaderException { > > ``` > > which calls `validateKey`, which can throw a `ClassCastException`.
Yeah, this code needs to be hardened a lot against various exceptions. There's a ticket for this (https://issues.apache.org/jira/browse/GEODE-3149), I've added a comment to this about this exception. > On July 13, 2017, 1:40 a.m., Galen O'Sullivan wrote: > > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandler.java > > Lines 38 (patched) > > <https://reviews.apache.org/r/60718/diff/4/?file=1775032#file1775032line38> > > > > I had thought of operation handlers as being stateless. Is this not the > > case? Hmm, now that you mention it, this will likely fail spectacularly the first time two threads run getAlls at the same time. Good catch. I'll see if Alexander and Udo can fix this as part of the refactoring work they're currently doing. > On July 13, 2017, 1:40 a.m., Galen O'Sullivan wrote: > > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandler.java > > Lines 59 (patched) > > <https://reviews.apache.org/r/60718/diff/4/?file=1775032#file1775032line59> > > > > This looks wrong. Shouldn't it be putting the keys somewhere? If the response message contains a PutResponse all key,values are assumed to have been successfully assigned. If we return an error, we can't make any guarantees about the state of any of the individual puts. If we can figure out how to get this information reliably back from the region PutAll request, we'll add a way to return it to the client. > On July 13, 2017, 1:40 a.m., Galen O'Sullivan wrote: > > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufResponseUtilities.java > > Line 40 (original), 38 (patched) > > <https://reviews.apache.org/r/60718/diff/4/?file=1775036#file1775036line40> > > > > Looks like we're creating error messages in this changeset, but not > > actually implementing the error message codes? That's correct, this is just the new format for ErrorResponses. We still need to figure out what the error codes are and provide utilities for setting them. > On July 13, 2017, 1:40 a.m., Galen O'Sullivan wrote: > > geode-protobuf/src/test/java/org/apache/geode/protocol/RoundTripCacheConnectionJUnitTest.java > > Lines 155 (patched) > > <https://reviews.apache.org/r/60718/diff/4/?file=1775041#file1775041line163> > > > > I wonder if it would make sense to extracted this block to a method in > > a `ProtobufTestUtilities` class, or if that's overkill. > > > > In general, these tests look really noisy to me, but I'm not sure what > > the best way to improve that would be. Ugh, this is all done in setup now as well. We're actually creating a new socket here which shadows the socket we should be using. This needs to be cleaned up. > On July 13, 2017, 1:40 a.m., Galen O'Sullivan wrote: > > geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/GetAllRequestOperationHandlerJUnitTest.java > > Lines 57 (patched) > > <https://reviews.apache.org/r/60718/diff/4/?file=1775042#file1775042line57> > > > > It seems to me that you're going to a lot of work to make the mock > > here. Would a spy be easier to understand? The mock is useful for injecting serialization errors to make sure they're properly handled. That being said, I'm not sure we're properly testing such conditions yet. > On July 13, 2017, 1:40 a.m., Galen O'Sullivan wrote: > > geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandlerJUnitTest.java > > Lines 68 (patched) > > <https://reviews.apache.org/r/60718/diff/4/?file=1775044#file1775044line68> > > > > The serialization service is stateless. Would a spy make more sense > > here? Similar to the GetAll, the mock allows us to test serialization errors. - Brian ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60718/#review180369 ----------------------------------------------------------- On July 12, 2017, 6:27 p.m., Brian Rowe wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60718/ > ----------------------------------------------------------- > > (Updated July 12, 2017, 6:27 p.m.) > > > Review request for geode, Alexander Murmann, Bruce Schuchardt, Galen > O'Sullivan, Hitesh Khamesra, and Udo Kohlmeyer. > > > Bugs: GEODE-2997 > https://issues.apache.org/jira/browse/GEODE-2997 > > > Repository: geode > > > Description > ------- > > Changed get response to indicate if LookupFailure was a missing key or key > with null value, added test > Added GetAllRequestOperationHandler and unit test > Added PutAllRequestOperationHandler and unit test > Added an integration test covering the putAll and getAll operations > > > Diffs > ----- > > > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufStreamProcessor.java > 714639274 > > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetAllRequestOperationHandler.java > PRE-CREATION > > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetRequestOperationHandler.java > 13b156f99 > > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandler.java > PRE-CREATION > > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/PutRequestOperationHandler.java > fecf01d7b > > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/RemoveRequestOperationHandler.java > e1fef85b4 > > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufRequestUtilities.java > b246a501b > > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufResponseUtilities.java > d6ef2788e > > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufUtilities.java > 924979329 > geode-protobuf/src/main/proto/clientProtocol.proto d94c0f312 > geode-protobuf/src/main/proto/region_API.proto 3108cb7c3 > geode-protobuf/src/test/java/org/apache/geode/protocol/MessageUtil.java > fee9448af > > geode-protobuf/src/test/java/org/apache/geode/protocol/RoundTripCacheConnectionJUnitTest.java > 612b9c9a4 > > geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/GetAllRequestOperationHandlerJUnitTest.java > PRE-CREATION > > geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/GetRequestOperationHandlerJUnitTest.java > b7d52019e > > geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandlerJUnitTest.java > PRE-CREATION > > > Diff: https://reviews.apache.org/r/60718/diff/4/ > > > Testing > ------- > > Added unit tests for new operation handlers > Added integration test covering new operations > > > Thanks, > > Brian Rowe > >
