> On July 6, 2017, 8:45 p.m., Udo Kohlmeyer wrote: > > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/PutRequestOperationHandler.java > > Line 32 (original), 34 (patched) > > <https://reviews.apache.org/r/60629/diff/1/?file=1768750#file1768750line35> > > > > 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.
Having the operation specific object in the API between the OpHandler and the OpProcessor really made the processor code a mess and made it impossible to add the error response (Galen and I spent an entire day trying to figure out how to make that work). I'm willing to try other approaches here, but the Request/Response types here make this code feel so much cleaner than the previous approach. We can discuss this more in person. > On July 6, 2017, 8:45 p.m., Udo Kohlmeyer wrote: > > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufResponseUtilities.java > > Lines 59 (patched) > > <https://reviews.apache.org/r/60629/diff/1/?file=1768753#file1768753line59> > > > > Why does the `ProtobufResponseUtility` require a specific method to log > > error responses? Should this not be part of the code. Galen felt that we should have a single call that would log the error and create the error object using the same message. I'm not as attached to this change, but we should have this discussion with him. - Brian ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60629/#review179796 ----------------------------------------------------------- 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 > >
