geode-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Udo Kohlmeyer <ukohlme...@gmail.com>
Subject Re: Review Request 60217: GEODE-2995: adding in protocol handler for protobuf messages
Date Wed, 21 Jun 2017 00:52:16 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message