geode-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Bruce Schuchardt <bschucha...@pivotal.io>
Subject Re: Review Request 60217: GEODE-2995: adding in protocol handler for protobuf messages
Date Tue, 20 Jun 2017 21:53:56 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60217/#review178410
-----------------------------------------------------------




geode-protobuf/src/main/java/org/apache/geode/ProtobufUtilities.java
Lines 22 (patched)
<https://reviews.apache.org/r/60217/#comment252368>

    The new classes are all missing javadocs



geode-protobuf/src/main/java/org/apache/geode/protocol/OpsProcessor.java
Lines 45 (patched)
<https://reviews.apache.org/r/60217/#comment252353>

    Create a Logger and use it to log a stack trace with information about the context of
the exception.
    
    private static final Logger logger = LogService.getLogger();



geode-protobuf/src/main/java/org/apache/geode/protocol/operations/OperationHandler.java
Lines 26 (patched)
<https://reviews.apache.org/r/60217/#comment252378>

    I think all interface methods should have javadocs



geode-protobuf/src/main/java/org/apache/geode/protocol/operations/ProtobufRequestOperationParser.java
Lines 29 (patched)
<https://reviews.apache.org/r/60217/#comment252377>

    Maybe InternalGemFireException would be better than RuntimeException here.



geode-protobuf/src/main/java/org/apache/geode/serialization/SerializationService.java
Lines 22 (patched)
<https://reviews.apache.org/r/60217/#comment252379>

    method javadocs please



geode-protobuf/src/main/java/org/apache/geode/serialization/registry/SerializationCodecRegistry.java
Lines 29 (patched)
<https://reviews.apache.org/r/60217/#comment252380>

    Since the collection of codecs is fixed by the enumeration why are you using a ServiceLoader
to scan the classpath for annotated codecs?  That seems inefficient.  I think this should
be removed until such time as the set of codecs is actually extensible.  It will be a lot
faster to just have a table of the supported codec classes and iterate over it.


- Bruce Schuchardt


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


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