----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60718/#review179967 -----------------------------------------------------------
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetAllRequestOperationHandler.java Lines 48-50 (patched) <https://reviews.apache.org/r/60718/#comment254933> I don't think we should return an error here... If they send us an empty list of keys, they get an empty response back. geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandler.java Lines 41 (patched) <https://reviews.apache.org/r/60718/#comment254935> I'm really not a big fan of a 60 line long method. Maybe we can break this into smaller methods? geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandler.java Lines 48-51 (patched) <https://reviews.apache.org/r/60718/#comment254934> Empty putAll does nothing. I don't believe we should log an error. geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandler.java Lines 81 (patched) <https://reviews.apache.org/r/60718/#comment254936> I'm sure that we can use `region.putAll(entries)` for this. geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufRequestUtilities.java Lines 76 (patched) <https://reviews.apache.org/r/60718/#comment254937> maybe this variable name should be `getAllRequestBuilder`... not advocate of generic `builder` geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufRequestUtilities.java Lines 78-80 (patched) <https://reviews.apache.org/r/60718/#comment254942> Could we replace this with `addAllKey(java.lang.Iterable<? extends org.apache.geode.protocol.protobuf.BasicTypes.EncodedValue> values)` geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufRequestUtilities.java Lines 95-97 (patched) <https://reviews.apache.org/r/60718/#comment254944> Could we replace this with `addAllEntry(java.lang.Iterable<? extends org.apache.geode.protocol.protobuf.BasicTypes.Entry> values)` geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufResponseUtilities.java Lines 125-127 (patched) <https://reviews.apache.org/r/60718/#comment254941> Could we replace this with `addAllEntries(java.lang.Iterable<? extends org.apache.geode.protocol.protobuf.BasicTypes.Entry> values)` geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufResponseUtilities.java Lines 141-143 (patched) <https://reviews.apache.org/r/60718/#comment254945> could we possibly use `addAllFailedKeys(java.lang.Iterable<? extends org.apache.geode.protocol.protobuf.BasicTypes.EncodedValue> values)` - Udo Kohlmeyer On July 7, 2017, 9:18 p.m., Brian Rowe wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60718/ > ----------------------------------------------------------- > > (Updated July 7, 2017, 9:18 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 > ebd5c6a0a > > 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/PutAllRequestOperationHandler.java > PRE-CREATION > > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufRequestUtilities.java > b96f478d1 > > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufResponseUtilities.java > 2114fdbf7 > > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufUtilities.java > 924979329 > > geode-protobuf/src/test/java/org/apache/geode/protocol/RoundTripCacheConnectionJUnitTest.java > 31a873658 > > 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/1/ > > > Testing > ------- > > Added unit tests for new operation handlers > Added integration test covering new operations > > > Thanks, > > Brian Rowe > >
