> On July 6, 2017, 8:45 p.m., Udo Kohlmeyer wrote: > > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufStreamProcessor.java > > Line 67 (original), 68 (patched) > > <https://reviews.apache.org/r/60629/diff/1/?file=1768746#file1768746line72> > > > > 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` > > Brian Rowe wrote: > This call specifically reads a single message from the incoming socket > and writes a single message to the outgoing socket. It's called > processOneMessage to make it clear that it's not handling multiple messages.
I prefer `processMessage`... but then again... we can hopefully refactor this out some day > On July 6, 2017, 8:45 p.m., Udo Kohlmeyer wrote: > > geode-protobuf/src/main/proto/clientProtocol.proto > > Lines 80 (patched) > > <https://reviews.apache.org/r/60629/diff/1/?file=1768756#file1768756line80> > > > > Why does a client care if it is a server error or not? What does this > > mean for a client? > > Brian Rowe wrote: > The error response fields were dictated by the PDD team, we should sit > down with them to define what the specific meaning of the fields are (as I'm > not even sure when I'm setting them). I think that we need to rethink the ErrorMessage and Error handling. > On July 6, 2017, 8:45 p.m., Udo Kohlmeyer wrote: > > geode-protobuf/src/main/proto/clientProtocol.proto > > Lines 81 (patched) > > <https://reviews.apache.org/r/60629/diff/1/?file=1768756#file1768756line81> > > > > 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` > > Brian Rowe wrote: > Same as previous response, we'll get this clarified by PDD. This is not something that is determined by the server. Clients should decide this for themselves. - Udo ----------------------------------------------------------- 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 > >
