----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60718/#review180369 -----------------------------------------------------------
Tests could use a little thoughtful addition of whitespace. Grouping related statements with empty lines between them helps a lot with readability. It would be nice to see some tests that used more varied datatypes, and I think this might be a good case for property-based (quickcheck) testing. Since I know y'all in person, let's chat in person about this. It would also be nice to see some tests of failure cases and malformed messages -- that may come later, but might be a good thing to do now. Maybe this can be tied in with custom generators for property-based testing. Let's talk about this too. geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetAllRequestOperationHandler.java Lines 59 (patched) <https://reviews.apache.org/r/60718/#comment255524> 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`. geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandler.java Lines 38 (patched) <https://reviews.apache.org/r/60718/#comment255530> I had thought of operation handlers as being stateless. Is this not the case? geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandler.java Lines 59 (patched) <https://reviews.apache.org/r/60718/#comment255529> This looks wrong. Shouldn't it be putting the keys somewhere? geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufResponseUtilities.java Line 40 (original), 38 (patched) <https://reviews.apache.org/r/60718/#comment255531> Looks like we're creating error messages in this changeset, but not actually implementing the error message codes? geode-protobuf/src/test/java/org/apache/geode/protocol/RoundTripCacheConnectionJUnitTest.java Lines 155 (patched) <https://reviews.apache.org/r/60718/#comment255532> 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. geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/GetAllRequestOperationHandlerJUnitTest.java Lines 57 (patched) <https://reviews.apache.org/r/60718/#comment255536> 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? geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandlerJUnitTest.java Lines 68 (patched) <https://reviews.apache.org/r/60718/#comment255537> The serialization service is stateless. Would a spy make more sense here? - Galen O'Sullivan 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 > >
