geode-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Brian Rowe <br...@pivotal.io>
Subject Re: Review Request 60629: GEODE-3129 - Added error messages to protobuf protocol
Date Fri, 07 Jul 2017 00:13:02 GMT


> On July 6, 2017, 8:45 p.m., Udo Kohlmeyer wrote:
> > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/PutRequestOperationHandler.java
> > Line 32 (original), 34 (patched)
> > <https://reviews.apache.org/r/60629/diff/1/?file=1768750#file1768750line35>
> >
> >     I don't agree with the `*Handlers` returning the `Response` objects.
> >     Imo, each `Handler` should only return the `Response` for the operation. It
should the job of something external to correctly wrap the method specific response into the
`Response` object.
> >     IF we are trying to avoid some code to switch on each message type, maybe an
abstract `ProtobufOperationHandler` is required, which introduces a real flow which will require
each implementation to correctly insert the operation specific.

Having the operation specific object in the API between the OpHandler and the OpProcessor
really made the processor code a mess and made it impossible to add the error response (Galen
and I spent an entire day trying to figure out how to make that work). I'm willing to try
other approaches here, but the Request/Response types here make this code feel so much cleaner
than the previous approach.  We can discuss this more in person.


> On July 6, 2017, 8:45 p.m., Udo Kohlmeyer wrote:
> > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufResponseUtilities.java
> > Lines 59 (patched)
> > <https://reviews.apache.org/r/60629/diff/1/?file=1768753#file1768753line59>
> >
> >     Why does the `ProtobufResponseUtility` require a specific method to log error
responses? Should this not be part of the code.

Galen felt that we should have a single call that would log the error and create the error
object using the same message. I'm not as attached to this change, but we should have this
discussion with him.


- Brian


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


On July 3, 2017, 11:40 p.m., Brian Rowe wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60629/
> -----------------------------------------------------------
> 
> (Updated July 3, 2017, 11:40 p.m.)
> 
> 
> Review request for geode, Alexander Murmann, Bruce Schuchardt, Galen O'Sullivan, Hitesh
Khamesra, and Udo Kohlmeyer.
> 
> 
> Bugs: GEODE-3129
>     https://issues.apache.org/jira/browse/GEODE-3129
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> added a new ErrorResponse type to ClientProtocol
> removed success field from several RegionAPI response objects and refactored operation
handlers to instead return ErrorResponses
> made all op handlers take ClientProtocol.Requests and return ClientProtocol.Responses
instead of operation specific types
> moved the protobuf specific response building code from operation handlers to ProtobufResponseUtilities
> moved the request building functions from MessageUtil (under Test) to ProtobufRequestUtilities
> moved all utility classes to ...protocol.protobuf.utilities and added javadoc comments
throughout
> changed GetRegions to GetRegionNames, returns strings instead of Regions
> replaced logging through the cache's LogWriter with log4j logging
> updated all imports to be in the correct order for the new geode style guide
> 
> 
> Diffs
> -----
> 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/operations/registry/OperationsHandlerRegistry.java
2b9f52597 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/EncodingTypeTranslator.java
edb7c7eb1 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufOpsProcessor.java
4318fb444 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufStreamProcessor.java
4e76de4a1 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufUtilities.java
c92da67a2 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetRegionsRequestOperationHandler.java
dc1d8acdd 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetRequestOperationHandler.java
95026e8d7 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/PutRequestOperationHandler.java
f375244d8 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/serializer/ProtobufProtocolSerializer.java
683e42f3f 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufRequestUtilities.java
PRE-CREATION 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufResponseUtilities.java
PRE-CREATION 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufUtilities.java
PRE-CREATION 
>   geode-protobuf/src/main/java/org/apache/geode/serialization/codec/IntCodec.java 6bd2b5c91

>   geode-protobuf/src/main/proto/clientProtocol.proto 0c192950a 
>   geode-protobuf/src/main/proto/region_API.proto d3af17acb 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/IntegrationJUnitTest.java 42cc7b3d0

>   geode-protobuf/src/test/java/org/apache/geode/protocol/MessageUtil.java dc897241f 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/RoundTripCacheConnectionJUnitTest.java
77b984f7e 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/operations/registry/OperationsHandlerRegistryJUnitTest.java
612e6a76f 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/EncodingTypeToSerializationTypeTranslatorJUnitTest.java
8e6f66aae 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/ProtobufOpsProcessorJUnitTest.java
c51be5cbb 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/GetRegionRequestOperationHandlerJUnitTest.java
e8f1e651a 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/GetRequestOperationHandlerJUnitTest.java
f92b1941a 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/PutRequestOperationHandlerJUnitTest.java
ddc23fc42 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/serializer/ProtobufProtocolSerializerJUnitTest.java
5a94dae01 
>   geode-protobuf/src/test/java/org/apache/geode/serialization/codec/BinaryFormatJUnitTest.java
dd72a190e 
> 
> 
> Diff: https://reviews.apache.org/r/60629/diff/1/
> 
> 
> Testing
> -------
> 
> Protobuf tests impacted by these changes have been refactored to check for error responses.
> 
> 
> Thanks,
> 
> Brian Rowe
> 
>


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