----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60217/#review178442 -----------------------------------------------------------
geode-protobuf/src/main/java/org/apache/geode/ProtobufUtilities.java Lines 22 (patched) <https://reviews.apache.org/r/60217/#comment252438> I think this should really be the start of a proper ProtobufMessageUtil class that will help with the building of Protobuf messages. This should hopefully hide the "horrible" builder pattern that would otherwise litter the code. geode-protobuf/src/main/java/org/apache/geode/ProtobufUtilities.java Lines 29 (patched) <https://reviews.apache.org/r/60217/#comment252437> Not sure why we have this (or ever had this). This is not functionality that we should have ever... What does it mean to have a default header? Each header should have meaning. I hope.... geode-protobuf/src/main/java/org/apache/geode/protocol/operations/registry/OperationsHandlerRegistry.java Lines 24 (patched) <https://reviews.apache.org/r/60217/#comment252436> It is hard to say if it is specific to protobuf. It is the mapping of an operation in the IDL to the corresponding handler. Would be interesting to see if we can find a simple way to maintain the mapping of IDL operation IDs and their OperationHandlers. geode-protobuf/src/test/java/org/apache/geode/client/protocol/MessageUtil.java Lines 20 (patched) <https://reviews.apache.org/r/60217/#comment252428> This utility is need not only in the tests but also when building a message to send back. We would not want to have the same boilerplate code in every message geode-protobuf/src/test/java/org/apache/geode/serialization/ProtobufSerializationServiceImplTest.java Lines 27 (patched) <https://reviews.apache.org/r/60217/#comment252434> I think the convention is *JUnitTest or *DUnitTest. We might have to go through all Test classes and amend accordingly. geode-protobuf/src/test/java/org/apache/geode/serialization/ProtobufSerializationServiceImplTest.java Lines 32 (patched) <https://reviews.apache.org/r/60217/#comment252431> I don't like these kinds of tests where we bulk test many different things. imo, I'd opt to have multiple tests for each type. At least I can see if different scenarios have been broken, by running the test once, rather than iteratively. geode-protobuf/src/test/java/org/apache/geode/serialization/codec/StringCodecJUnitTest.java Lines 29 (patched) <https://reviews.apache.org/r/60217/#comment252432> Given now have SerializationServiceImplTest, does this still make sense? - Udo Kohlmeyer On June 21, 2017, 12:02 a.m., Brian Rowe wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60217/ > ----------------------------------------------------------- > > (Updated June 21, 2017, 12:02 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/protocol/exception/InvalidProtocolMessageException.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/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/protocol/protobuf/EncodingTypeTranslator.java > PRE-CREATION > > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufOpsProcessor.java > PRE-CREATION > > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufSerializationService.java > PRE-CREATION > > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufStreamProcessor.java > PRE-CREATION > > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufUtilities.java > PRE-CREATION > > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetRequestOperationHandler.java > PRE-CREATION > > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/serializer/ProtobufProtocolSerializer.java > PRE-CREATION > > geode-protobuf/src/main/java/org/apache/geode/protocol/serializer/ProtocolSerializer.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/TypeEncodingException.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/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.operations.OperationHandler > PRE-CREATION > > geode-protobuf/src/main/resources/META-INF/services/org.apache.geode.protocol.serializer.ProtocolSerializer > 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/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/protocol/protobuf/ProtobufOpsProcessorTest.java > PRE-CREATION > > geode-protobuf/src/test/java/org/apache/geode/protocol/serializer/ProtobufProtocolSerializerJUnitTest.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/2/ > > > Testing > ------- > > Precheckin in progress. Unit tests added for all new classes, integration > test added for entire module. > > > Thanks, > > Brian Rowe > >
