----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60629/#review179796 -----------------------------------------------------------
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufStreamProcessor.java Line 67 (original), 68 (patched) <https://reviews.apache.org/r/60629/#comment254699> Why do we have `processOneMessage`? How does this method ensure that only a single message is processed. It is misleading and makes no sense. Think we should stick to `processMessage` geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/PutRequestOperationHandler.java Line 32 (original), 34 (patched) <https://reviews.apache.org/r/60629/#comment254698> I don't agree with the `*Handlers` returning the `Response` objects. Imo, each `Handler` should only return the `Response` for the operation. It should the job of something external to correctly wrap the method specific response into the `Response` object. IF we are trying to avoid some code to switch on each message type, maybe an abstract `ProtobufOperationHandler` is required, which introduces a real flow which will require each implementation to correctly insert the operation specific. geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufRequestUtilities.java Lines 57 (patched) <https://reviews.apache.org/r/60629/#comment254696> Misleading method name. It is not returning a `PutRequest` but rather a `Request` object. geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufResponseUtilities.java Lines 59 (patched) <https://reviews.apache.org/r/60629/#comment254695> Why does the `ProtobufResponseUtility` require a specific method to log error responses? Should this not be part of the code. geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufResponseUtilities.java Lines 101 (patched) <https://reviews.apache.org/r/60629/#comment254692> This is misleading. This is returning a `Response` object rather than a `GetRegionNamesResponse` object. geode-protobuf/src/main/proto/clientProtocol.proto Line 53 (original), 53 (patched) <https://reviews.apache.org/r/60629/#comment254684> How about `GetAvailableRegions`? geode-protobuf/src/main/proto/clientProtocol.proto Lines 80 (patched) <https://reviews.apache.org/r/60629/#comment254685> Why does a client care if it is a server error or not? What does this mean for a client? geode-protobuf/src/main/proto/clientProtocol.proto Lines 81 (patched) <https://reviews.apache.org/r/60629/#comment254686> How does one determine if it is `retriable` or not? Surely every type of error should be seen as a failure rather than `try again later` geode-protobuf/src/main/proto/region_API.proto Lines 36-38 (original), 42-45 (patched) <https://reviews.apache.org/r/60629/#comment254687> This should really still only be an `EncodedValue`. Special casing the `GetResponse` payload should not be required. geode-protobuf/src/main/proto/region_API.proto Line 37 (original), 43 (patched) <https://reviews.apache.org/r/60629/#comment254641> I don't think we need this. Too many conditional checks. Maybe we pass back a `null` for the `result`. geode-protobuf/src/main/proto/region_API.proto Line 102 (original), 109 (patched) <https://reviews.apache.org/r/60629/#comment254689> As raised earlier, how about `GetAvailableRegions`? - Udo Kohlmeyer On July 3, 2017, 11:40 p.m., Brian Rowe wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60629/ > ----------------------------------------------------------- > > (Updated July 3, 2017, 11:40 p.m.) > > > Review request for geode, Alexander Murmann, Bruce Schuchardt, Galen > O'Sullivan, Hitesh Khamesra, and Udo Kohlmeyer. > > > Bugs: GEODE-3129 > https://issues.apache.org/jira/browse/GEODE-3129 > > > Repository: geode > > > Description > ------- > > added a new ErrorResponse type to ClientProtocol > removed success field from several RegionAPI response objects and refactored > operation handlers to instead return ErrorResponses > made all op handlers take ClientProtocol.Requests and return > ClientProtocol.Responses instead of operation specific types > moved the protobuf specific response building code from operation handlers to > ProtobufResponseUtilities > moved the request building functions from MessageUtil (under Test) to > ProtobufRequestUtilities > moved all utility classes to ...protocol.protobuf.utilities and added javadoc > comments throughout > changed GetRegions to GetRegionNames, returns strings instead of Regions > replaced logging through the cache's LogWriter with log4j logging > updated all imports to be in the correct order for the new geode style guide > > > Diffs > ----- > > > geode-protobuf/src/main/java/org/apache/geode/protocol/operations/registry/OperationsHandlerRegistry.java > 2b9f52597 > > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/EncodingTypeTranslator.java > edb7c7eb1 > > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufOpsProcessor.java > 4318fb444 > > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufStreamProcessor.java > 4e76de4a1 > > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufUtilities.java > c92da67a2 > > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetRegionsRequestOperationHandler.java > dc1d8acdd > > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetRequestOperationHandler.java > 95026e8d7 > > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/PutRequestOperationHandler.java > f375244d8 > > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/serializer/ProtobufProtocolSerializer.java > 683e42f3f > > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufRequestUtilities.java > PRE-CREATION > > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufResponseUtilities.java > PRE-CREATION > > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufUtilities.java > PRE-CREATION > > geode-protobuf/src/main/java/org/apache/geode/serialization/codec/IntCodec.java > 6bd2b5c91 > geode-protobuf/src/main/proto/clientProtocol.proto 0c192950a > geode-protobuf/src/main/proto/region_API.proto d3af17acb > > geode-protobuf/src/test/java/org/apache/geode/protocol/IntegrationJUnitTest.java > 42cc7b3d0 > geode-protobuf/src/test/java/org/apache/geode/protocol/MessageUtil.java > dc897241f > > geode-protobuf/src/test/java/org/apache/geode/protocol/RoundTripCacheConnectionJUnitTest.java > 77b984f7e > > geode-protobuf/src/test/java/org/apache/geode/protocol/operations/registry/OperationsHandlerRegistryJUnitTest.java > 612e6a76f > > geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/EncodingTypeToSerializationTypeTranslatorJUnitTest.java > 8e6f66aae > > geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/ProtobufOpsProcessorJUnitTest.java > c51be5cbb > > geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/GetRegionRequestOperationHandlerJUnitTest.java > e8f1e651a > > geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/GetRequestOperationHandlerJUnitTest.java > f92b1941a > > geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/PutRequestOperationHandlerJUnitTest.java > ddc23fc42 > > geode-protobuf/src/test/java/org/apache/geode/protocol/serializer/ProtobufProtocolSerializerJUnitTest.java > 5a94dae01 > > geode-protobuf/src/test/java/org/apache/geode/serialization/codec/BinaryFormatJUnitTest.java > dd72a190e > > > Diff: https://reviews.apache.org/r/60629/diff/1/ > > > Testing > ------- > > Protobuf tests impacted by these changes have been refactored to check for > error responses. > > > Thanks, > > Brian Rowe > >
