----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60217/#review178412 -----------------------------------------------------------
geode-protobuf/src/main/java/org/apache/geode/protocol/OpsProcessor.java Lines 45 (patched) <https://reviews.apache.org/r/60217/#comment252355> Good catch, we shouldn't be catching anything here. geode-protobuf/src/main/java/org/apache/geode/protocol/handler/ProtobufStreamProcessor.java Lines 35 (patched) <https://reviews.apache.org/r/60217/#comment252357> It looks like the only state being maintained in here is the mapping of op code to op handler and for serialization type to codec, both of which are set up at construction time. There shouldn't be any problem calling into this from multiple threads. geode-protobuf/src/main/java/org/apache/geode/protocol/handler/ProtobufStreamProcessor.java Lines 49 (patched) <https://reviews.apache.org/r/60217/#comment252370> Moved this to ...protocol.protobuf. Within that package I created protobuf.handler and protobuf.operations and moved in the protobuf classes from org.apache.geode.protocol.{handler,operations}.protobuf to keep all the protobuf code in one area. geode-protobuf/src/main/java/org/apache/geode/protocol/handler/ProtocolHandler.java Lines 23 (patched) <https://reviews.apache.org/r/60217/#comment252371> Good call, ProtocolSerializer is much more descriptive. Changed. geode-protobuf/src/main/java/org/apache/geode/protocol/operations/OperationHandler.java Lines 26 (patched) <https://reviews.apache.org/r/60217/#comment252372> This is only used during the constructor of the OperationHandlerRegistry so that it can register itself. We'll probably remove the service loading code in the registry construction, which will lead to the removal of this as well. geode-protobuf/src/main/java/org/apache/geode/protocol/operations/ProtobufRequestOperationParser.java Lines 29 (patched) <https://reviews.apache.org/r/60217/#comment252373> Good call, changed this to InvalidProtocolMessageException. geode-protobuf/src/main/java/org/apache/geode/protocol/operations/registry/OperationsHandlerRegistry.java Lines 24 (patched) <https://reviews.apache.org/r/60217/#comment252374> It's actually only specific to the protobuf because the service loader will automatically register all of the protobuf operation handlers. Let's remove service loading here and just create a protobuf specific instance in the ProtobufStreamProcessor which will individually register the protobuf specific handlers. Given that this is somewhat broader scope than the rest of the review changes, I'm going to leave this for a follow up push after the merge. geode-protobuf/src/test/java/org/apache/geode/client/protocol/IntegrationTest.java Lines 66 (patched) <https://reviews.apache.org/r/60217/#comment252375> How about testGetRequestProcessed? geode-protobuf/src/test/java/org/apache/geode/client/protocol/IntegrationTest.java Lines 67 (patched) <https://reviews.apache.org/r/60217/#comment252376> That's a lot simpler, thanks! geode-protobuf/src/test/java/org/apache/geode/client/protocol/IntegrationTest.java Lines 82 (patched) <https://reviews.apache.org/r/60217/#comment252381> Turn out this is pretty easy to check through Mockito, added. geode-protobuf/src/test/java/org/apache/geode/protocol/handler/ProtobufProtocolHandlerJUnitTest.java Lines 62 (patched) <https://reviews.apache.org/r/60217/#comment252382> Nice, this really cleans up the test a lot geode-protobuf/src/test/java/org/apache/geode/serialization/ProtobufSerializationServiceImplTest.java Lines 54 (patched) <https://reviews.apache.org/r/60217/#comment252383> Fixed geode-protobuf/src/test/java/org/apache/geode/serialization/registry/CodecRegistryJUnitTest.java Lines 35 (patched) <https://reviews.apache.org/r/60217/#comment252384> Without this, JUnit can't determine the category of this test - http://static.javadoc.io/org.powermock/powermock-core/1.6.5/org/powermock/core/classloader/annotations/PowerMockIgnore.html geode-protobuf/src/test/java/org/apache/geode/serialization/registry/CodecRegistryJUnitTest.java Lines 58 (patched) <https://reviews.apache.org/r/60217/#comment252385> fixed settings.gradle Line 39 (original), 39 (patched) <https://reviews.apache.org/r/60217/#comment252354> Alright, I'll leave that for that batch of work then - Brian Rowe 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 > >
