> 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`
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. > On July 6, 2017, 8:45 p.m., Udo Kohlmeyer wrote: > > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufRequestUtilities.java > > Lines 57 (patched) > > <https://reviews.apache.org/r/60629/diff/1/?file=1768752#file1768752line57> > > > > Misleading method name. It is not returning a `PutRequest` but rather a > > `Request` object. That was intentional as the PutRequest is not generally useful outside of a Request object. Do you think there is value in a function that returns just a PutRequest, or do you just object to the name (what would you propose as an alternative?)? > 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 101 (patched) > > <https://reviews.apache.org/r/60629/diff/1/?file=1768753#file1768753line101> > > > > This is misleading. This is returning a `Response` object rather than a > > `GetRegionNamesResponse` object. Same as two comments previous, we can discuss this futher in person. > On July 6, 2017, 8:45 p.m., Udo Kohlmeyer wrote: > > geode-protobuf/src/main/proto/clientProtocol.proto > > Line 53 (original), 53 (patched) > > <https://reviews.apache.org/r/60629/diff/1/?file=1768756#file1768756line53> > > > > How about `GetAvailableRegions`? The main goal here was just to make it differentiated from GetRegion (which is apparently the API that PDD actually wants). I do feel that getRegionNames is a bit more accurate since this returns just a list of the names of regions, rather than the entire region objects. > 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? 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). > 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` Same as previous response, we'll get this clarified by PDD. > On July 6, 2017, 8:45 p.m., Udo Kohlmeyer wrote: > > geode-protobuf/src/main/proto/region_API.proto > > Lines 36-38 (original), 42-45 (patched) > > <https://reviews.apache.org/r/60629/diff/1/?file=1768757#file1768757line42> > > > > This should really still only be an `EncodedValue`. Special casing the > > `GetResponse` payload should not be required. Agreed, I didn't realize that we could just have a null EncodedValue. > On July 6, 2017, 8:45 p.m., Udo Kohlmeyer wrote: > > geode-protobuf/src/main/proto/region_API.proto > > Line 37 (original), 43 (patched) > > <https://reviews.apache.org/r/60629/diff/1/?file=1768757#file1768757line43> > > > > I don't think we need this. Too many conditional checks. Maybe we pass > > back a `null` for the `result`. No, null EncodedValue is sufficient. Removed this field. > On July 6, 2017, 8:45 p.m., Udo Kohlmeyer wrote: > > geode-protobuf/src/main/proto/region_API.proto > > Line 102 (original), 109 (patched) > > <https://reviews.apache.org/r/60629/diff/1/?file=1768757#file1768757line109> > > > > As raised earlier, how about `GetAvailableRegions`? See my response to that comment. - 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 > >
