----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60217/#review178404 -----------------------------------------------------------
Good work! I would love to see more doc comments in general. It can be hard to figure out the meaning of a class just from looking at the code. I've gotten as far as the first test; I will look at the rest later. geode-protobuf/src/main/java/org/apache/geode/protocol/OpsProcessor.java Lines 45 (patched) <https://reviews.apache.org/r/60217/#comment252340> We'll throw a NPE if the exception is thrown anyways, would it make more sense to rethrow here? geode-protobuf/src/main/java/org/apache/geode/protocol/handler/ProtobufStreamProcessor.java Lines 35 (patched) <https://reviews.apache.org/r/60217/#comment252341> Is this thread-safe? I'd like to have one copy on the server side and call processOneMessage from any thread. In theory the message processors and such shouldn't store any state. Do you agree? Do you think it's safe to share one `ProtobufStreamProcessor` between threads and sockets? geode-protobuf/src/main/java/org/apache/geode/protocol/handler/ProtobufStreamProcessor.java Lines 49 (patched) <https://reviews.apache.org/r/60217/#comment252342> Should this maybe use the `org.apache.geode.protocol.protobuf.handler` or similar namespace? It looks protobuf-specific. geode-protobuf/src/main/java/org/apache/geode/protocol/handler/ProtocolHandler.java Lines 23 (patched) <https://reviews.apache.org/r/60217/#comment252343> Would `ProtocolSerializer` or similar be a clearer name for this? Handle kind of sounds like it does message processing beyond serialization. geode-protobuf/src/main/java/org/apache/geode/protocol/operations/OperationHandler.java Lines 26 (patched) <https://reviews.apache.org/r/60217/#comment252344> What does this function do? It could use a doc comment. The interface could use a doc comment as well. geode-protobuf/src/main/java/org/apache/geode/protocol/operations/ProtobufRequestOperationParser.java Lines 29 (patched) <https://reviews.apache.org/r/60217/#comment252346> Should it be a `RuntimeException` for us to get a malformed request? I would have thought of this as being more like an `IOException` or an `InvalidProtocolMessageException`. geode-protobuf/src/main/java/org/apache/geode/protocol/operations/registry/OperationsHandlerRegistry.java Lines 24 (patched) <https://reviews.apache.org/r/60217/#comment252348> This looks like it's specific to protobuf, so I would like to see it in a protobuf package. Because it's specific to protobuf, I don't think it makes that much sense to be a global using a `ServiceLoader`. geode-protobuf/src/test/java/org/apache/geode/client/protocol/IntegrationTest.java Lines 66 (patched) <https://reviews.apache.org/r/60217/#comment252351> can we call this `testGetRequestProcessor()`? geode-protobuf/src/test/java/org/apache/geode/client/protocol/IntegrationTest.java Lines 67 (patched) <https://reviews.apache.org/r/60217/#comment252349> Especially since this is a test, I think it's clearer to write `throws Exception` here. geode-protobuf/src/test/java/org/apache/geode/client/protocol/IntegrationTest.java Lines 82 (patched) <https://reviews.apache.org/r/60217/#comment252350> We could assert here that we've gotten the region exactly once and/or we've only gotten the one key. It's not a lot of benefit, though. settings.gradle Line 39 (original), 39 (patched) <https://reviews.apache.org/r/60217/#comment252339> `geode-protobuf` will also need to be added to `geode-assembly` before this is functional, but we can do that in the work that combines this with the `ServerConnection` work. - Galen O'Sullivan On June 20, 2017, 12:17 a.m., Brian Rowe wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60217/ > ----------------------------------------------------------- > > (Updated June 20, 2017, 12:17 a.m.) > > > Review request for geode, Alexander Murmann, Bruce Schuchardt, Galen > O'Sullivan, Hitesh Khamesra, and Udo Kohlmeyer. > > > Repository: geode > > > Description > ------- > > This change adds a new module for handling client stresms encoded using the > new ProtoBuf protocol. At the top level this can be integrated by passing in > the input/output streams and cache reference to the ProtobufStreamProcessor. > This will decode the message and ultimately dispatch it to an operation > specific handler which will call back into the passed cache object. Note > that this not currently hooked up to geode at all, GEODE-3075 is tracking the > work needed to hook this up at that level. > > This change mainly contains the plumbing and encoding/decoding logic, but > also contain the Get operation handler as a proof of concept. Future work > will be needed to handle other types of operations. > > > Diffs > ----- > > geode-protobuf/build.gradle PRE-CREATION > geode-protobuf/src/main/java/org/apache/geode/ProtobufUtilities.java > PRE-CREATION > geode-protobuf/src/main/java/org/apache/geode/protocol/OpsProcessor.java > PRE-CREATION > > geode-protobuf/src/main/java/org/apache/geode/protocol/exception/InvalidProtocolMessageException.java > PRE-CREATION > > geode-protobuf/src/main/java/org/apache/geode/protocol/handler/ProtobufStreamProcessor.java > PRE-CREATION > > geode-protobuf/src/main/java/org/apache/geode/protocol/handler/ProtocolHandler.java > PRE-CREATION > > geode-protobuf/src/main/java/org/apache/geode/protocol/handler/protobuf/ProtobufProtocolHandler.java > PRE-CREATION > > geode-protobuf/src/main/java/org/apache/geode/protocol/operations/OperationHandler.java > PRE-CREATION > > geode-protobuf/src/main/java/org/apache/geode/protocol/operations/ProtobufRequestOperationParser.java > PRE-CREATION > > geode-protobuf/src/main/java/org/apache/geode/protocol/operations/protobuf/GetRequestOperationHandler.java > PRE-CREATION > > geode-protobuf/src/main/java/org/apache/geode/protocol/operations/registry/OperationsHandlerRegistry.java > PRE-CREATION > > geode-protobuf/src/main/java/org/apache/geode/protocol/operations/registry/exception/OperationHandlerAlreadyRegisteredException.java > PRE-CREATION > > geode-protobuf/src/main/java/org/apache/geode/protocol/operations/registry/exception/OperationHandlerNotRegisteredException.java > PRE-CREATION > > geode-protobuf/src/main/java/org/apache/geode/serialization/ProtobufSerializationService.java > PRE-CREATION > > geode-protobuf/src/main/java/org/apache/geode/serialization/SerializationService.java > PRE-CREATION > > geode-protobuf/src/main/java/org/apache/geode/serialization/SerializationType.java > PRE-CREATION > geode-protobuf/src/main/java/org/apache/geode/serialization/TypeCodec.java > PRE-CREATION > > geode-protobuf/src/main/java/org/apache/geode/serialization/codec/BinaryCodec.java > PRE-CREATION > > geode-protobuf/src/main/java/org/apache/geode/serialization/codec/BooleanCodec.java > PRE-CREATION > > geode-protobuf/src/main/java/org/apache/geode/serialization/codec/ByteCodec.java > PRE-CREATION > > geode-protobuf/src/main/java/org/apache/geode/serialization/codec/DoubleCodec.java > PRE-CREATION > > geode-protobuf/src/main/java/org/apache/geode/serialization/codec/FloatCodec.java > PRE-CREATION > > geode-protobuf/src/main/java/org/apache/geode/serialization/codec/IntCodec.java > PRE-CREATION > > geode-protobuf/src/main/java/org/apache/geode/serialization/codec/JSONCodec.java > PRE-CREATION > > geode-protobuf/src/main/java/org/apache/geode/serialization/codec/LongCodec.java > PRE-CREATION > > geode-protobuf/src/main/java/org/apache/geode/serialization/codec/ShortCodec.java > PRE-CREATION > > geode-protobuf/src/main/java/org/apache/geode/serialization/codec/StringCodec.java > PRE-CREATION > > geode-protobuf/src/main/java/org/apache/geode/serialization/exception/UnsupportedEncodingTypeException.java > PRE-CREATION > > geode-protobuf/src/main/java/org/apache/geode/serialization/protobuf/translation/EncodingTypeTranslator.java > PRE-CREATION > > geode-protobuf/src/main/java/org/apache/geode/serialization/protobuf/translation/exception/UnsupportedEncodingTypeException.java > PRE-CREATION > > geode-protobuf/src/main/java/org/apache/geode/serialization/registry/SerializationCodecRegistry.java > PRE-CREATION > > geode-protobuf/src/main/java/org/apache/geode/serialization/registry/exception/CodecAlreadyRegisteredForTypeException.java > PRE-CREATION > > geode-protobuf/src/main/java/org/apache/geode/serialization/registry/exception/CodecNotRegisteredForTypeException.java > PRE-CREATION > geode-protobuf/src/main/proto/basicTypes.proto PRE-CREATION > geode-protobuf/src/main/proto/clientProtocol.proto PRE-CREATION > geode-protobuf/src/main/proto/region_API.proto PRE-CREATION > geode-protobuf/src/main/proto/server_API.proto PRE-CREATION > > geode-protobuf/src/main/resources/META-INF/services/org.apache.geode.protocol.handler.ProtocolHandler > PRE-CREATION > > geode-protobuf/src/main/resources/META-INF/services/org.apache.geode.protocol.operations.OperationHandler > PRE-CREATION > > geode-protobuf/src/main/resources/META-INF/services/org.apache.geode.serialization.TypeCodec > PRE-CREATION > > geode-protobuf/src/test/java/org/apache/geode/client/protocol/IntegrationTest.java > PRE-CREATION > > geode-protobuf/src/test/java/org/apache/geode/client/protocol/MessageUtil.java > PRE-CREATION > > geode-protobuf/src/test/java/org/apache/geode/client/protocol/OpsHandler.java > PRE-CREATION > > geode-protobuf/src/test/java/org/apache/geode/client/protocol/OpsProcessorTest.java > PRE-CREATION > > geode-protobuf/src/test/java/org/apache/geode/protocol/handler/ProtobufProtocolHandlerJUnitTest.java > PRE-CREATION > > geode-protobuf/src/test/java/org/apache/geode/protocol/operations/protobuf/GetRequestOperationHandlerTest.java > PRE-CREATION > > geode-protobuf/src/test/java/org/apache/geode/protocol/operations/registry/OperationsHandlerRegistryJUnitTest.java > PRE-CREATION > > geode-protobuf/src/test/java/org/apache/geode/serialization/ProtobufSerializationServiceImplTest.java > PRE-CREATION > > geode-protobuf/src/test/java/org/apache/geode/serialization/codec/StringCodecJUnitTest.java > PRE-CREATION > > geode-protobuf/src/test/java/org/apache/geode/serialization/protobuf/translation/EncodingTypeToSerializationTypeTranslatorJUnitTest.java > PRE-CREATION > > geode-protobuf/src/test/java/org/apache/geode/serialization/registry/CodecRegistryJUnitTest.java > PRE-CREATION > gradle/rat.gradle 1bea5843b > settings.gradle c0fdb6e4f > > > Diff: https://reviews.apache.org/r/60217/diff/1/ > > > Testing > ------- > > Precheckin in progress. Unit tests added for all new classes, integration > test added for entire module. > > > Thanks, > > Brian Rowe > >
