You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@geode.apache.org by Brian Rowe <br...@pivotal.io> on 2017/07/07 21:18:39 UTC

Review Request 60718: GEODE-2997: New flow getAll/putAll

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

Review request for geode, Alexander Murmann, Bruce Schuchardt, Galen O'Sullivan, Hitesh Khamesra, and Udo Kohlmeyer.


Bugs: GEODE-2997
    https://issues.apache.org/jira/browse/GEODE-2997


Repository: geode


Description
-------

Changed get response to indicate if LookupFailure was a missing key or key with null value, added test
Added GetAllRequestOperationHandler and unit test
Added PutAllRequestOperationHandler and unit test
Added an integration test covering the putAll and getAll operations


Diffs
-----

  geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufStreamProcessor.java ebd5c6a0a 
  geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetAllRequestOperationHandler.java PRE-CREATION 
  geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandler.java PRE-CREATION 
  geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufRequestUtilities.java b96f478d1 
  geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufResponseUtilities.java 2114fdbf7 
  geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufUtilities.java 924979329 
  geode-protobuf/src/test/java/org/apache/geode/protocol/RoundTripCacheConnectionJUnitTest.java 31a873658 
  geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/GetAllRequestOperationHandlerJUnitTest.java PRE-CREATION 
  geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/GetRequestOperationHandlerJUnitTest.java b7d52019e 
  geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandlerJUnitTest.java PRE-CREATION 


Diff: https://reviews.apache.org/r/60718/diff/1/


Testing
-------

Added unit tests for new operation handlers
Added integration test covering new operations


Thanks,

Brian Rowe

Re: Review Request 60718: GEODE-2997: New flow getAll/putAll

Posted by Brian Rowe <br...@pivotal.io>.


> On July 7, 2017, 11:51 p.m., Udo Kohlmeyer wrote:
> > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetAllRequestOperationHandler.java
> > Lines 48-50 (patched)
> > <https://reviews.apache.org/r/60718/diff/1/?file=1771593#file1771593line48>
> >
> >     I don't think we should return an error here... If they send us an empty list of keys, they get an empty response back.

Fixed


> On July 7, 2017, 11:51 p.m., Udo Kohlmeyer wrote:
> > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandler.java
> > Lines 41 (patched)
> > <https://reviews.apache.org/r/60718/diff/1/?file=1771594#file1771594line41>
> >
> >     I'm really not a big fan of a 60 line long method. Maybe we can break this into smaller methods?

Broke out several sub-methods.


> On July 7, 2017, 11:51 p.m., Udo Kohlmeyer wrote:
> > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandler.java
> > Lines 48-51 (patched)
> > <https://reviews.apache.org/r/60718/diff/1/?file=1771594#file1771594line48>
> >
> >     Empty putAll does nothing. I don't believe we should log an error.

Fixed.


- Brian


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


On July 7, 2017, 9:18 p.m., Brian Rowe wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60718/
> -----------------------------------------------------------
> 
> (Updated July 7, 2017, 9:18 p.m.)
> 
> 
> Review request for geode, Alexander Murmann, Bruce Schuchardt, Galen O'Sullivan, Hitesh Khamesra, and Udo Kohlmeyer.
> 
> 
> Bugs: GEODE-2997
>     https://issues.apache.org/jira/browse/GEODE-2997
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> Changed get response to indicate if LookupFailure was a missing key or key with null value, added test
> Added GetAllRequestOperationHandler and unit test
> Added PutAllRequestOperationHandler and unit test
> Added an integration test covering the putAll and getAll operations
> 
> 
> Diffs
> -----
> 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufStreamProcessor.java ebd5c6a0a 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetAllRequestOperationHandler.java PRE-CREATION 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandler.java PRE-CREATION 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufRequestUtilities.java b96f478d1 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufResponseUtilities.java 2114fdbf7 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufUtilities.java 924979329 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/RoundTripCacheConnectionJUnitTest.java 31a873658 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/GetAllRequestOperationHandlerJUnitTest.java PRE-CREATION 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/GetRequestOperationHandlerJUnitTest.java b7d52019e 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandlerJUnitTest.java PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60718/diff/1/
> 
> 
> Testing
> -------
> 
> Added unit tests for new operation handlers
> Added integration test covering new operations
> 
> 
> Thanks,
> 
> Brian Rowe
> 
>

Re: Review Request 60718: GEODE-2997: New flow getAll/putAll

Posted by Brian Rowe <br...@pivotal.io>.

We will still return ErrorResponses for any failures or invalid messages,
we've just removed the serverInternal and retriable fields from this
message in favor a errorCode integer.

On Wed, Jul 12, 2017 at 11:45 AM, Michael Stolz <ms...@pivotal.io> wrote:

> We removed error feedback?
> So how is an application programmer supposed to determine what failed now?
> Without that information we may have rendered putAll unusable for some
> cases.
>
> --
> Mike Stolz
> Principal Engineer, GemFire Product Manager
> Mobile: +1-631-835-4771 <(631)%20835-4771>
>
> On Wed, Jul 12, 2017 at 1:45 PM, Brian Rowe <br...@pivotal.io> wrote:
>
>>
>>
>> > On July 7, 2017, 11:51 p.m., Udo Kohlmeyer wrote:
>> > > geode-protobuf/src/main/java/org/apache/geode/protocol/proto
>> buf/operations/PutAllRequestOperationHandler.java
>> > > Lines 81 (patched)
>> > > <https://reviews.apache.org/r/60718/diff/1/?file=1771594#fil
>> e1771594line81>
>> > >
>> > >     I'm sure that we can use `region.putAll(entries)` for this.
>>
>> Fixed.  This also resulted in us not being able to determine the state of
>> a partially succeeded PutAll, so we updated the PutAllResponse to remove
>> the failedKeys field.
>>
>>
>> > On July 7, 2017, 11:51 p.m., Udo Kohlmeyer wrote:
>> > > geode-protobuf/src/main/java/org/apache/geode/protocol/proto
>> buf/utilities/ProtobufRequestUtilities.java
>> > > Lines 76 (patched)
>> > > <https://reviews.apache.org/r/60718/diff/1/?file=1771595#fil
>> e1771595line76>
>> > >
>> > >     maybe this variable name should be `getAllRequestBuilder`... not
>> advocate of generic `builder`
>>
>> Fixed
>>
>>
>> > On July 7, 2017, 11:51 p.m., Udo Kohlmeyer wrote:
>> > > geode-protobuf/src/main/java/org/apache/geode/protocol/proto
>> buf/utilities/ProtobufRequestUtilities.java
>> > > Lines 78-80 (patched)
>> > > <https://reviews.apache.org/r/60718/diff/1/?file=1771595#fil
>> e1771595line78>
>> > >
>> > >     Could we replace this with `addAllKey(java.lang.Iterable<?
>> extends org.apache.geode.protocol.protobuf.BasicTypes.EncodedValue>
>> values)`
>>
>> Nice, good catch.
>>
>>
>> > On July 7, 2017, 11:51 p.m., Udo Kohlmeyer wrote:
>> > > geode-protobuf/src/main/java/org/apache/geode/protocol/proto
>> buf/utilities/ProtobufRequestUtilities.java
>> > > Lines 95-97 (patched)
>> > > <https://reviews.apache.org/r/60718/diff/1/?file=1771595#fil
>> e1771595line95>
>> > >
>> > >     Could we replace this with `addAllEntry(java.lang.Iterable<?
>> extends org.apache.geode.protocol.protobuf.BasicTypes.Entry> values)`
>>
>> Fixed
>>
>>
>> > On July 7, 2017, 11:51 p.m., Udo Kohlmeyer wrote:
>> > > geode-protobuf/src/main/java/org/apache/geode/protocol/proto
>> buf/utilities/ProtobufResponseUtilities.java
>> > > Lines 125-127 (patched)
>> > > <https://reviews.apache.org/r/60718/diff/1/?file=1771596#fil
>> e1771596line125>
>> > >
>> > >     Could we replace this with `addAllEntries(java.lang.Iterable<?
>> extends org.apache.geode.protocol.protobuf.BasicTypes.Entry> values)`
>>
>> Fixed
>>
>>
>> > On July 7, 2017, 11:51 p.m., Udo Kohlmeyer wrote:
>> > > geode-protobuf/src/main/java/org/apache/geode/protocol/proto
>> buf/utilities/ProtobufResponseUtilities.java
>> > > Lines 141-143 (patched)
>> > > <https://reviews.apache.org/r/60718/diff/1/?file=1771596#fil
>> e1771596line141>
>> > >
>> > >     could we possibly use `addAllFailedKeys(java.lang.Iterable<?
>> extends org.apache.geode.protocol.protobuf.BasicTypes.EncodedValue>
>> values)`
>>
>> This got removed with the failed keys field.
>>
>>
>> - Brian
>>
>>
>> -----------------------------------------------------------
>> This is an automatically generated e-mail. To reply, visit:
>> https://reviews.apache.org/r/60718/#review179967
>> -----------------------------------------------------------
>>
>>
>> On July 7, 2017, 9:18 p.m., Brian Rowe wrote:
>> >
>> > -----------------------------------------------------------
>> > This is an automatically generated e-mail. To reply, visit:
>> > https://reviews.apache.org/r/60718/
>> > -----------------------------------------------------------
>> >
>> > (Updated July 7, 2017, 9:18 p.m.)
>> >
>> >
>> > Review request for geode, Alexander Murmann, Bruce Schuchardt, Galen
>> O'Sullivan, Hitesh Khamesra, and Udo Kohlmeyer.
>> >
>> >
>> > Bugs: GEODE-2997
>> >     https://issues.apache.org/jira/browse/GEODE-2997
>> >
>> >
>> > Repository: geode
>> >
>> >
>> > Description
>> > -------
>> >
>> > Changed get response to indicate if LookupFailure was a missing key or
>> key with null value, added test
>> > Added GetAllRequestOperationHandler and unit test
>> > Added PutAllRequestOperationHandler and unit test
>> > Added an integration test covering the putAll and getAll operations
>> >
>> >
>> > Diffs
>> > -----
>> >
>> >   geode-protobuf/src/main/java/org/apache/geode/protocol/prot
>> obuf/ProtobufStreamProcessor.java ebd5c6a0a
>> >   geode-protobuf/src/main/java/org/apache/geode/protocol/prot
>> obuf/operations/GetAllRequestOperationHandler.java PRE-CREATION
>> >   geode-protobuf/src/main/java/org/apache/geode/protocol/prot
>> obuf/operations/PutAllRequestOperationHandler.java PRE-CREATION
>> >   geode-protobuf/src/main/java/org/apache/geode/protocol/prot
>> obuf/utilities/ProtobufRequestUtilities.java b96f478d1
>> >   geode-protobuf/src/main/java/org/apache/geode/protocol/prot
>> obuf/utilities/ProtobufResponseUtilities.java 2114fdbf7
>> >   geode-protobuf/src/main/java/org/apache/geode/protocol/prot
>> obuf/utilities/ProtobufUtilities.java 924979329
>> >   geode-protobuf/src/test/java/org/apache/geode/protocol/Roun
>> dTripCacheConnectionJUnitTest.java 31a873658
>> >   geode-protobuf/src/test/java/org/apache/geode/protocol/prot
>> obuf/operations/GetAllRequestOperationHandlerJUnitTest.java PRE-CREATION
>> >   geode-protobuf/src/test/java/org/apache/geode/protocol/prot
>> obuf/operations/GetRequestOperationHandlerJUnitTest.java b7d52019e
>> >   geode-protobuf/src/test/java/org/apache/geode/protocol/prot
>> obuf/operations/PutAllRequestOperationHandlerJUnitTest.java PRE-CREATION
>> >
>> >
>> > Diff: https://reviews.apache.org/r/60718/diff/1/
>> >
>> >
>> > Testing
>> > -------
>> >
>> > Added unit tests for new operation handlers
>> > Added integration test covering new operations
>> >
>> >
>> > Thanks,
>> >
>> > Brian Rowe
>> >
>> >
>>
>>
>

Re: Review Request 60718: GEODE-2997: New flow getAll/putAll

Posted by Michael Stolz <ms...@pivotal.io>.

We removed error feedback?
So how is an application programmer supposed to determine what failed now?
Without that information we may have rendered putAll unusable for some
cases.

--
Mike Stolz
Principal Engineer, GemFire Product Manager
Mobile: +1-631-835-4771

On Wed, Jul 12, 2017 at 1:45 PM, Brian Rowe <br...@pivotal.io> wrote:

>
>
> > On July 7, 2017, 11:51 p.m., Udo Kohlmeyer wrote:
> > > geode-protobuf/src/main/java/org/apache/geode/protocol/
> protobuf/operations/PutAllRequestOperationHandler.java
> > > Lines 81 (patched)
> > > <https://reviews.apache.org/r/60718/diff/1/?file=1771594#
> file1771594line81>
> > >
> > >     I'm sure that we can use `region.putAll(entries)` for this.
>
> Fixed.  This also resulted in us not being able to determine the state of
> a partially succeeded PutAll, so we updated the PutAllResponse to remove
> the failedKeys field.
>
>
> > On July 7, 2017, 11:51 p.m., Udo Kohlmeyer wrote:
> > > geode-protobuf/src/main/java/org/apache/geode/protocol/
> protobuf/utilities/ProtobufRequestUtilities.java
> > > Lines 76 (patched)
> > > <https://reviews.apache.org/r/60718/diff/1/?file=1771595#
> file1771595line76>
> > >
> > >     maybe this variable name should be `getAllRequestBuilder`... not
> advocate of generic `builder`
>
> Fixed
>
>
> > On July 7, 2017, 11:51 p.m., Udo Kohlmeyer wrote:
> > > geode-protobuf/src/main/java/org/apache/geode/protocol/
> protobuf/utilities/ProtobufRequestUtilities.java
> > > Lines 78-80 (patched)
> > > <https://reviews.apache.org/r/60718/diff/1/?file=1771595#
> file1771595line78>
> > >
> > >     Could we replace this with `addAllKey(java.lang.Iterable<?
> extends org.apache.geode.protocol.protobuf.BasicTypes.EncodedValue>
> values)`
>
> Nice, good catch.
>
>
> > On July 7, 2017, 11:51 p.m., Udo Kohlmeyer wrote:
> > > geode-protobuf/src/main/java/org/apache/geode/protocol/
> protobuf/utilities/ProtobufRequestUtilities.java
> > > Lines 95-97 (patched)
> > > <https://reviews.apache.org/r/60718/diff/1/?file=1771595#
> file1771595line95>
> > >
> > >     Could we replace this with `addAllEntry(java.lang.Iterable<?
> extends org.apache.geode.protocol.protobuf.BasicTypes.Entry> values)`
>
> Fixed
>
>
> > On July 7, 2017, 11:51 p.m., Udo Kohlmeyer wrote:
> > > geode-protobuf/src/main/java/org/apache/geode/protocol/
> protobuf/utilities/ProtobufResponseUtilities.java
> > > Lines 125-127 (patched)
> > > <https://reviews.apache.org/r/60718/diff/1/?file=1771596#
> file1771596line125>
> > >
> > >     Could we replace this with `addAllEntries(java.lang.Iterable<?
> extends org.apache.geode.protocol.protobuf.BasicTypes.Entry> values)`
>
> Fixed
>
>
> > On July 7, 2017, 11:51 p.m., Udo Kohlmeyer wrote:
> > > geode-protobuf/src/main/java/org/apache/geode/protocol/
> protobuf/utilities/ProtobufResponseUtilities.java
> > > Lines 141-143 (patched)
> > > <https://reviews.apache.org/r/60718/diff/1/?file=1771596#
> file1771596line141>
> > >
> > >     could we possibly use `addAllFailedKeys(java.lang.Iterable<?
> extends org.apache.geode.protocol.protobuf.BasicTypes.EncodedValue>
> values)`
>
> This got removed with the failed keys field.
>
>
> - Brian
>
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60718/#review179967
> -----------------------------------------------------------
>
>
> On July 7, 2017, 9:18 p.m., Brian Rowe wrote:
> >
> > -----------------------------------------------------------
> > This is an automatically generated e-mail. To reply, visit:
> > https://reviews.apache.org/r/60718/
> > -----------------------------------------------------------
> >
> > (Updated July 7, 2017, 9:18 p.m.)
> >
> >
> > Review request for geode, Alexander Murmann, Bruce Schuchardt, Galen
> O'Sullivan, Hitesh Khamesra, and Udo Kohlmeyer.
> >
> >
> > Bugs: GEODE-2997
> >     https://issues.apache.org/jira/browse/GEODE-2997
> >
> >
> > Repository: geode
> >
> >
> > Description
> > -------
> >
> > Changed get response to indicate if LookupFailure was a missing key or
> key with null value, added test
> > Added GetAllRequestOperationHandler and unit test
> > Added PutAllRequestOperationHandler and unit test
> > Added an integration test covering the putAll and getAll operations
> >
> >
> > Diffs
> > -----
> >
> >   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufStreamProcessor.java
> ebd5c6a0a
> >   geode-protobuf/src/main/java/org/apache/geode/protocol/
> protobuf/operations/GetAllRequestOperationHandler.java PRE-CREATION
> >   geode-protobuf/src/main/java/org/apache/geode/protocol/
> protobuf/operations/PutAllRequestOperationHandler.java PRE-CREATION
> >   geode-protobuf/src/main/java/org/apache/geode/protocol/
> protobuf/utilities/ProtobufRequestUtilities.java b96f478d1
> >   geode-protobuf/src/main/java/org/apache/geode/protocol/
> protobuf/utilities/ProtobufResponseUtilities.java 2114fdbf7
> >   geode-protobuf/src/main/java/org/apache/geode/protocol/
> protobuf/utilities/ProtobufUtilities.java 924979329
> >   geode-protobuf/src/test/java/org/apache/geode/protocol/
> RoundTripCacheConnectionJUnitTest.java 31a873658
> >   geode-protobuf/src/test/java/org/apache/geode/protocol/
> protobuf/operations/GetAllRequestOperationHandlerJUnitTest.java
> PRE-CREATION
> >   geode-protobuf/src/test/java/org/apache/geode/protocol/
> protobuf/operations/GetRequestOperationHandlerJUnitTest.java b7d52019e
> >   geode-protobuf/src/test/java/org/apache/geode/protocol/
> protobuf/operations/PutAllRequestOperationHandlerJUnitTest.java
> PRE-CREATION
> >
> >
> > Diff: https://reviews.apache.org/r/60718/diff/1/
> >
> >
> > Testing
> > -------
> >
> > Added unit tests for new operation handlers
> > Added integration test covering new operations
> >
> >
> > Thanks,
> >
> > Brian Rowe
> >
> >
>
>

Re: Review Request 60718: GEODE-2997: New flow getAll/putAll

Posted by Brian Rowe <br...@pivotal.io>.


> On July 7, 2017, 11:51 p.m., Udo Kohlmeyer wrote:
> > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandler.java
> > Lines 81 (patched)
> > <https://reviews.apache.org/r/60718/diff/1/?file=1771594#file1771594line81>
> >
> >     I'm sure that we can use `region.putAll(entries)` for this.

Fixed.  This also resulted in us not being able to determine the state of a partially succeeded PutAll, so we updated the PutAllResponse to remove the failedKeys field.


> On July 7, 2017, 11:51 p.m., Udo Kohlmeyer wrote:
> > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufRequestUtilities.java
> > Lines 76 (patched)
> > <https://reviews.apache.org/r/60718/diff/1/?file=1771595#file1771595line76>
> >
> >     maybe this variable name should be `getAllRequestBuilder`... not advocate of generic `builder`

Fixed


> On July 7, 2017, 11:51 p.m., Udo Kohlmeyer wrote:
> > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufRequestUtilities.java
> > Lines 78-80 (patched)
> > <https://reviews.apache.org/r/60718/diff/1/?file=1771595#file1771595line78>
> >
> >     Could we replace this with `addAllKey(java.lang.Iterable<? extends org.apache.geode.protocol.protobuf.BasicTypes.EncodedValue> values)`

Nice, good catch.


> On July 7, 2017, 11:51 p.m., Udo Kohlmeyer wrote:
> > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufRequestUtilities.java
> > Lines 95-97 (patched)
> > <https://reviews.apache.org/r/60718/diff/1/?file=1771595#file1771595line95>
> >
> >     Could we replace this with `addAllEntry(java.lang.Iterable<? extends org.apache.geode.protocol.protobuf.BasicTypes.Entry> values)`

Fixed


> On July 7, 2017, 11:51 p.m., Udo Kohlmeyer wrote:
> > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufResponseUtilities.java
> > Lines 125-127 (patched)
> > <https://reviews.apache.org/r/60718/diff/1/?file=1771596#file1771596line125>
> >
> >     Could we replace this with `addAllEntries(java.lang.Iterable<? extends org.apache.geode.protocol.protobuf.BasicTypes.Entry> values)`

Fixed


> On July 7, 2017, 11:51 p.m., Udo Kohlmeyer wrote:
> > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufResponseUtilities.java
> > Lines 141-143 (patched)
> > <https://reviews.apache.org/r/60718/diff/1/?file=1771596#file1771596line141>
> >
> >     could we possibly use `addAllFailedKeys(java.lang.Iterable<? extends org.apache.geode.protocol.protobuf.BasicTypes.EncodedValue> values)`

This got removed with the failed keys field.


- Brian


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


On July 7, 2017, 9:18 p.m., Brian Rowe wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60718/
> -----------------------------------------------------------
> 
> (Updated July 7, 2017, 9:18 p.m.)
> 
> 
> Review request for geode, Alexander Murmann, Bruce Schuchardt, Galen O'Sullivan, Hitesh Khamesra, and Udo Kohlmeyer.
> 
> 
> Bugs: GEODE-2997
>     https://issues.apache.org/jira/browse/GEODE-2997
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> Changed get response to indicate if LookupFailure was a missing key or key with null value, added test
> Added GetAllRequestOperationHandler and unit test
> Added PutAllRequestOperationHandler and unit test
> Added an integration test covering the putAll and getAll operations
> 
> 
> Diffs
> -----
> 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufStreamProcessor.java ebd5c6a0a 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetAllRequestOperationHandler.java PRE-CREATION 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandler.java PRE-CREATION 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufRequestUtilities.java b96f478d1 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufResponseUtilities.java 2114fdbf7 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufUtilities.java 924979329 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/RoundTripCacheConnectionJUnitTest.java 31a873658 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/GetAllRequestOperationHandlerJUnitTest.java PRE-CREATION 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/GetRequestOperationHandlerJUnitTest.java b7d52019e 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandlerJUnitTest.java PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60718/diff/1/
> 
> 
> Testing
> -------
> 
> Added unit tests for new operation handlers
> Added integration test covering new operations
> 
> 
> Thanks,
> 
> Brian Rowe
> 
>

Re: Review Request 60718: GEODE-2997: New flow getAll/putAll

Posted by Udo Kohlmeyer <uk...@gmail.com>.

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




geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetAllRequestOperationHandler.java
Lines 48-50 (patched)
<https://reviews.apache.org/r/60718/#comment254933>

    I don't think we should return an error here... If they send us an empty list of keys, they get an empty response back.



geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandler.java
Lines 41 (patched)
<https://reviews.apache.org/r/60718/#comment254935>

    I'm really not a big fan of a 60 line long method. Maybe we can break this into smaller methods?



geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandler.java
Lines 48-51 (patched)
<https://reviews.apache.org/r/60718/#comment254934>

    Empty putAll does nothing. I don't believe we should log an error.



geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandler.java
Lines 81 (patched)
<https://reviews.apache.org/r/60718/#comment254936>

    I'm sure that we can use `region.putAll(entries)` for this.



geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufRequestUtilities.java
Lines 76 (patched)
<https://reviews.apache.org/r/60718/#comment254937>

    maybe this variable name should be `getAllRequestBuilder`... not advocate of generic `builder`



geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufRequestUtilities.java
Lines 78-80 (patched)
<https://reviews.apache.org/r/60718/#comment254942>

    Could we replace this with `addAllKey(java.lang.Iterable<? extends org.apache.geode.protocol.protobuf.BasicTypes.EncodedValue> values)`



geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufRequestUtilities.java
Lines 95-97 (patched)
<https://reviews.apache.org/r/60718/#comment254944>

    Could we replace this with `addAllEntry(java.lang.Iterable<? extends org.apache.geode.protocol.protobuf.BasicTypes.Entry> values)`



geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufResponseUtilities.java
Lines 125-127 (patched)
<https://reviews.apache.org/r/60718/#comment254941>

    Could we replace this with `addAllEntries(java.lang.Iterable<? extends org.apache.geode.protocol.protobuf.BasicTypes.Entry> values)`



geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufResponseUtilities.java
Lines 141-143 (patched)
<https://reviews.apache.org/r/60718/#comment254945>

    could we possibly use `addAllFailedKeys(java.lang.Iterable<? extends org.apache.geode.protocol.protobuf.BasicTypes.EncodedValue> values)`


- Udo Kohlmeyer


On July 7, 2017, 9:18 p.m., Brian Rowe wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60718/
> -----------------------------------------------------------
> 
> (Updated July 7, 2017, 9:18 p.m.)
> 
> 
> Review request for geode, Alexander Murmann, Bruce Schuchardt, Galen O'Sullivan, Hitesh Khamesra, and Udo Kohlmeyer.
> 
> 
> Bugs: GEODE-2997
>     https://issues.apache.org/jira/browse/GEODE-2997
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> Changed get response to indicate if LookupFailure was a missing key or key with null value, added test
> Added GetAllRequestOperationHandler and unit test
> Added PutAllRequestOperationHandler and unit test
> Added an integration test covering the putAll and getAll operations
> 
> 
> Diffs
> -----
> 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufStreamProcessor.java ebd5c6a0a 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetAllRequestOperationHandler.java PRE-CREATION 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandler.java PRE-CREATION 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufRequestUtilities.java b96f478d1 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufResponseUtilities.java 2114fdbf7 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufUtilities.java 924979329 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/RoundTripCacheConnectionJUnitTest.java 31a873658 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/GetAllRequestOperationHandlerJUnitTest.java PRE-CREATION 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/GetRequestOperationHandlerJUnitTest.java b7d52019e 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandlerJUnitTest.java PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60718/diff/1/
> 
> 
> Testing
> -------
> 
> Added unit tests for new operation handlers
> Added integration test covering new operations
> 
> 
> Thanks,
> 
> Brian Rowe
> 
>

Re: Review Request 60718: GEODE-2997: New flow getAll/putAll

Posted by Brian Rowe <br...@pivotal.io>.


> On July 12, 2017, 8:07 p.m., Bruce Schuchardt wrote:
> > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetAllRequestOperationHandler.java
> > Lines 55 (patched)
> > <https://reviews.apache.org/r/60718/diff/4/?file=1775030#file1775030line55>
> >
> >     I think we should change getAll to send an ordered collection of keys.  Then the server only has to respond with the corresponding values & doesn't have to send the keys back to the client.  This is what the current java client getAll command does.

Good idea, but this will be a fairly dramatic change at this point.  We're going to create a new bug for this and then later on discuss this change with everyone and potentially implement it then.


> On July 12, 2017, 8:07 p.m., Bruce Schuchardt wrote:
> > geode-protobuf/src/test/java/org/apache/geode/protocol/RoundTripCacheConnectionJUnitTest.java
> > Line 30 (original), 29 (patched)
> > <https://reviews.apache.org/r/60718/diff/4/?file=1775041#file1775041line30>
> >
> >     We're not supposed to use wildcard imports
> >     
> >     https://google.github.io/styleguide/javaguide.html#s3.3.1-wildcard-imports
> >     
> >     You can configure IntelliJ to do this by setting # of imports that triggers use of wildcard to 99.

Fixed.


- Brian


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


On July 12, 2017, 6:27 p.m., Brian Rowe wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60718/
> -----------------------------------------------------------
> 
> (Updated July 12, 2017, 6:27 p.m.)
> 
> 
> Review request for geode, Alexander Murmann, Bruce Schuchardt, Galen O'Sullivan, Hitesh Khamesra, and Udo Kohlmeyer.
> 
> 
> Bugs: GEODE-2997
>     https://issues.apache.org/jira/browse/GEODE-2997
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> Changed get response to indicate if LookupFailure was a missing key or key with null value, added test
> Added GetAllRequestOperationHandler and unit test
> Added PutAllRequestOperationHandler and unit test
> Added an integration test covering the putAll and getAll operations
> 
> 
> Diffs
> -----
> 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufStreamProcessor.java 714639274 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetAllRequestOperationHandler.java PRE-CREATION 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetRequestOperationHandler.java 13b156f99 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandler.java PRE-CREATION 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/PutRequestOperationHandler.java fecf01d7b 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/RemoveRequestOperationHandler.java e1fef85b4 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufRequestUtilities.java b246a501b 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufResponseUtilities.java d6ef2788e 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufUtilities.java 924979329 
>   geode-protobuf/src/main/proto/clientProtocol.proto d94c0f312 
>   geode-protobuf/src/main/proto/region_API.proto 3108cb7c3 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/MessageUtil.java fee9448af 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/RoundTripCacheConnectionJUnitTest.java 612b9c9a4 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/GetAllRequestOperationHandlerJUnitTest.java PRE-CREATION 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/GetRequestOperationHandlerJUnitTest.java b7d52019e 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandlerJUnitTest.java PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60718/diff/4/
> 
> 
> Testing
> -------
> 
> Added unit tests for new operation handlers
> Added integration test covering new operations
> 
> 
> Thanks,
> 
> Brian Rowe
> 
>

Re: Review Request 60718: GEODE-2997: New flow getAll/putAll

Posted by Bruce Schuchardt <bs...@pivotal.io>.

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




geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetAllRequestOperationHandler.java
Lines 55 (patched)
<https://reviews.apache.org/r/60718/#comment255478>

    I think we should change getAll to send an ordered collection of keys.  Then the server only has to respond with the corresponding values & doesn't have to send the keys back to the client.  This is what the current java client getAll command does.



geode-protobuf/src/test/java/org/apache/geode/protocol/RoundTripCacheConnectionJUnitTest.java
Line 30 (original), 29 (patched)
<https://reviews.apache.org/r/60718/#comment255479>

    We're not supposed to use wildcard imports
    
    https://google.github.io/styleguide/javaguide.html#s3.3.1-wildcard-imports
    
    You can configure IntelliJ to do this by setting # of imports that triggers use of wildcard to 99.


- Bruce Schuchardt


On July 12, 2017, 11:27 a.m., Brian Rowe wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60718/
> -----------------------------------------------------------
> 
> (Updated July 12, 2017, 11:27 a.m.)
> 
> 
> Review request for geode, Alexander Murmann, Bruce Schuchardt, Galen O'Sullivan, Hitesh Khamesra, and Udo Kohlmeyer.
> 
> 
> Bugs: GEODE-2997
>     https://issues.apache.org/jira/browse/GEODE-2997
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> Changed get response to indicate if LookupFailure was a missing key or key with null value, added test
> Added GetAllRequestOperationHandler and unit test
> Added PutAllRequestOperationHandler and unit test
> Added an integration test covering the putAll and getAll operations
> 
> 
> Diffs
> -----
> 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufStreamProcessor.java 714639274 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetAllRequestOperationHandler.java PRE-CREATION 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetRequestOperationHandler.java 13b156f99 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandler.java PRE-CREATION 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/PutRequestOperationHandler.java fecf01d7b 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/RemoveRequestOperationHandler.java e1fef85b4 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufRequestUtilities.java b246a501b 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufResponseUtilities.java d6ef2788e 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufUtilities.java 924979329 
>   geode-protobuf/src/main/proto/clientProtocol.proto d94c0f312 
>   geode-protobuf/src/main/proto/region_API.proto 3108cb7c3 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/MessageUtil.java fee9448af 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/RoundTripCacheConnectionJUnitTest.java 612b9c9a4 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/GetAllRequestOperationHandlerJUnitTest.java PRE-CREATION 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/GetRequestOperationHandlerJUnitTest.java b7d52019e 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandlerJUnitTest.java PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60718/diff/4/
> 
> 
> Testing
> -------
> 
> Added unit tests for new operation handlers
> Added integration test covering new operations
> 
> 
> Thanks,
> 
> Brian Rowe
> 
>

Re: Review Request 60718: GEODE-2997: New flow getAll/putAll

Posted by Udo Kohlmeyer <uk...@gmail.com>.

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


Ship it!




Ship It!

- Udo Kohlmeyer


On July 12, 2017, 6:27 p.m., Brian Rowe wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60718/
> -----------------------------------------------------------
> 
> (Updated July 12, 2017, 6:27 p.m.)
> 
> 
> Review request for geode, Alexander Murmann, Bruce Schuchardt, Galen O'Sullivan, Hitesh Khamesra, and Udo Kohlmeyer.
> 
> 
> Bugs: GEODE-2997
>     https://issues.apache.org/jira/browse/GEODE-2997
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> Changed get response to indicate if LookupFailure was a missing key or key with null value, added test
> Added GetAllRequestOperationHandler and unit test
> Added PutAllRequestOperationHandler and unit test
> Added an integration test covering the putAll and getAll operations
> 
> 
> Diffs
> -----
> 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufStreamProcessor.java 714639274 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetAllRequestOperationHandler.java PRE-CREATION 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetRequestOperationHandler.java 13b156f99 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandler.java PRE-CREATION 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/PutRequestOperationHandler.java fecf01d7b 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/RemoveRequestOperationHandler.java e1fef85b4 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufRequestUtilities.java b246a501b 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufResponseUtilities.java d6ef2788e 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufUtilities.java 924979329 
>   geode-protobuf/src/main/proto/clientProtocol.proto d94c0f312 
>   geode-protobuf/src/main/proto/region_API.proto 3108cb7c3 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/MessageUtil.java fee9448af 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/RoundTripCacheConnectionJUnitTest.java 612b9c9a4 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/GetAllRequestOperationHandlerJUnitTest.java PRE-CREATION 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/GetRequestOperationHandlerJUnitTest.java b7d52019e 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandlerJUnitTest.java PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60718/diff/4/
> 
> 
> Testing
> -------
> 
> Added unit tests for new operation handlers
> Added integration test covering new operations
> 
> 
> Thanks,
> 
> Brian Rowe
> 
>

Re: Review Request 60718: GEODE-2997: New flow getAll/putAll

Posted by Brian Rowe <br...@pivotal.io>.


> On July 13, 2017, 1:40 a.m., Galen O'Sullivan wrote:
> > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetAllRequestOperationHandler.java
> > Lines 59 (patched)
> > <https://reviews.apache.org/r/60718/diff/4/?file=1775030#file1775030line59>
> >
> >     It looks like this will throw `ClassCastException` if the key type doesn't satisfy key constraints. `LocalRegion`'s implementation calls `basicGetAll` to `accessEntry` to `validateKey`.
> >     
> >     I also followed `DataSetRegion`'s implementation through the code to 
> >     ```
> >     LocalRegion.get(Object key, Object aCallbackArgument, boolean generateCallbacks,
> >           boolean disableCopyOnRead, boolean preferCD, ClientProxyMembershipID requestingClient,
> >           EntryEventImpl clientEvent, boolean returnTombstones, boolean opScopeIsLocal,
> >           boolean retainResult) throws TimeoutException, CacheLoaderException {
> >     ```
> >     which calls `validateKey`, which can throw a `ClassCastException`.

Yeah, this code needs to be hardened a lot against various exceptions. There's a ticket for this (https://issues.apache.org/jira/browse/GEODE-3149), I've added a comment to this about this exception.


> On July 13, 2017, 1:40 a.m., Galen O'Sullivan wrote:
> > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandler.java
> > Lines 38 (patched)
> > <https://reviews.apache.org/r/60718/diff/4/?file=1775032#file1775032line38>
> >
> >     I had thought of operation handlers as being stateless. Is this not the case?

Hmm, now that you mention it, this will likely fail spectacularly the first time two threads run getAlls at the same time.  Good catch.  I'll see if Alexander and Udo can fix this as part of the refactoring work they're currently doing.


> On July 13, 2017, 1:40 a.m., Galen O'Sullivan wrote:
> > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandler.java
> > Lines 59 (patched)
> > <https://reviews.apache.org/r/60718/diff/4/?file=1775032#file1775032line59>
> >
> >     This looks wrong. Shouldn't it be putting the keys somewhere?

If the response message contains a PutResponse all key,values are assumed to have been successfully assigned. If we return an error, we can't make any guarantees about the state of any of the individual puts. If we can figure out how to get this information reliably back from the region PutAll request, we'll add a way to return it to the client.


> On July 13, 2017, 1:40 a.m., Galen O'Sullivan wrote:
> > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufResponseUtilities.java
> > Line 40 (original), 38 (patched)
> > <https://reviews.apache.org/r/60718/diff/4/?file=1775036#file1775036line40>
> >
> >     Looks like we're creating error messages in this changeset, but not actually implementing the error message codes?

That's correct, this is just the new format for ErrorResponses.  We still need to figure out what the error codes are and provide utilities for setting them.


> On July 13, 2017, 1:40 a.m., Galen O'Sullivan wrote:
> > geode-protobuf/src/test/java/org/apache/geode/protocol/RoundTripCacheConnectionJUnitTest.java
> > Lines 155 (patched)
> > <https://reviews.apache.org/r/60718/diff/4/?file=1775041#file1775041line163>
> >
> >     I wonder if it would make sense to extracted this block to a method in a `ProtobufTestUtilities` class, or if that's overkill.
> >     
> >     In general, these tests look really noisy to me, but I'm not sure what the best way to improve that would be.

Ugh, this is all done in setup now as well. We're actually creating a new socket here which shadows the socket we should be using. This needs to be cleaned up.


> On July 13, 2017, 1:40 a.m., Galen O'Sullivan wrote:
> > geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/GetAllRequestOperationHandlerJUnitTest.java
> > Lines 57 (patched)
> > <https://reviews.apache.org/r/60718/diff/4/?file=1775042#file1775042line57>
> >
> >     It seems to me that you're going to a lot of work to make the mock here. Would a spy be easier to understand?

The mock is useful for injecting serialization errors to make sure they're properly handled.  That being said, I'm not sure we're properly testing such conditions yet.


> On July 13, 2017, 1:40 a.m., Galen O'Sullivan wrote:
> > geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandlerJUnitTest.java
> > Lines 68 (patched)
> > <https://reviews.apache.org/r/60718/diff/4/?file=1775044#file1775044line68>
> >
> >     The serialization service is stateless. Would a spy make more sense here?

Similar to the GetAll, the mock allows us to test serialization errors.


- Brian


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


On July 12, 2017, 6:27 p.m., Brian Rowe wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60718/
> -----------------------------------------------------------
> 
> (Updated July 12, 2017, 6:27 p.m.)
> 
> 
> Review request for geode, Alexander Murmann, Bruce Schuchardt, Galen O'Sullivan, Hitesh Khamesra, and Udo Kohlmeyer.
> 
> 
> Bugs: GEODE-2997
>     https://issues.apache.org/jira/browse/GEODE-2997
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> Changed get response to indicate if LookupFailure was a missing key or key with null value, added test
> Added GetAllRequestOperationHandler and unit test
> Added PutAllRequestOperationHandler and unit test
> Added an integration test covering the putAll and getAll operations
> 
> 
> Diffs
> -----
> 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufStreamProcessor.java 714639274 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetAllRequestOperationHandler.java PRE-CREATION 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetRequestOperationHandler.java 13b156f99 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandler.java PRE-CREATION 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/PutRequestOperationHandler.java fecf01d7b 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/RemoveRequestOperationHandler.java e1fef85b4 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufRequestUtilities.java b246a501b 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufResponseUtilities.java d6ef2788e 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufUtilities.java 924979329 
>   geode-protobuf/src/main/proto/clientProtocol.proto d94c0f312 
>   geode-protobuf/src/main/proto/region_API.proto 3108cb7c3 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/MessageUtil.java fee9448af 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/RoundTripCacheConnectionJUnitTest.java 612b9c9a4 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/GetAllRequestOperationHandlerJUnitTest.java PRE-CREATION 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/GetRequestOperationHandlerJUnitTest.java b7d52019e 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandlerJUnitTest.java PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60718/diff/4/
> 
> 
> Testing
> -------
> 
> Added unit tests for new operation handlers
> Added integration test covering new operations
> 
> 
> Thanks,
> 
> Brian Rowe
> 
>

Re: Review Request 60718: GEODE-2997: New flow getAll/putAll

Posted by Alexander Murmann <am...@pivotal.io>.


> On July 13, 2017, 1:40 a.m., Galen O'Sullivan wrote:
> > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandler.java
> > Lines 38 (patched)
> > <https://reviews.apache.org/r/60718/diff/4/?file=1775032#file1775032line38>
> >
> >     I had thought of operation handlers as being stateless. Is this not the case?
> 
> Brian Rowe wrote:
>     Hmm, now that you mention it, this will likely fail spectacularly the first time two threads run getAlls at the same time.  Good catch.  I'll see if Alexander and Udo can fix this as part of the refactoring work they're currently doing.

From a code maintainability perspective I'd rather have us make a fresh handler for each request rather than having something that's supposed to be used like a singleton but isn't actually a singleton. Turning this into a singleton is also something I'd rather avoid.


- Alexander


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


On July 12, 2017, 6:27 p.m., Brian Rowe wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60718/
> -----------------------------------------------------------
> 
> (Updated July 12, 2017, 6:27 p.m.)
> 
> 
> Review request for geode, Alexander Murmann, Bruce Schuchardt, Galen O'Sullivan, Hitesh Khamesra, and Udo Kohlmeyer.
> 
> 
> Bugs: GEODE-2997
>     https://issues.apache.org/jira/browse/GEODE-2997
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> Changed get response to indicate if LookupFailure was a missing key or key with null value, added test
> Added GetAllRequestOperationHandler and unit test
> Added PutAllRequestOperationHandler and unit test
> Added an integration test covering the putAll and getAll operations
> 
> 
> Diffs
> -----
> 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufStreamProcessor.java 714639274 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetAllRequestOperationHandler.java PRE-CREATION 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetRequestOperationHandler.java 13b156f99 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandler.java PRE-CREATION 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/PutRequestOperationHandler.java fecf01d7b 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/RemoveRequestOperationHandler.java e1fef85b4 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufRequestUtilities.java b246a501b 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufResponseUtilities.java d6ef2788e 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufUtilities.java 924979329 
>   geode-protobuf/src/main/proto/clientProtocol.proto d94c0f312 
>   geode-protobuf/src/main/proto/region_API.proto 3108cb7c3 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/MessageUtil.java fee9448af 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/RoundTripCacheConnectionJUnitTest.java 612b9c9a4 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/GetAllRequestOperationHandlerJUnitTest.java PRE-CREATION 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/GetRequestOperationHandlerJUnitTest.java b7d52019e 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandlerJUnitTest.java PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60718/diff/4/
> 
> 
> Testing
> -------
> 
> Added unit tests for new operation handlers
> Added integration test covering new operations
> 
> 
> Thanks,
> 
> Brian Rowe
> 
>

Re: Review Request 60718: GEODE-2997: New flow getAll/putAll

Posted by Galen O'Sullivan <go...@pivotal.io>.

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



Tests could use a little thoughtful addition of whitespace. Grouping related statements with empty lines between them helps a lot with readability.

It would be nice to see some tests that used more varied datatypes, and I think this might be a good case for property-based (quickcheck) testing. Since I know y'all in person, let's chat in person about this.
It would also be nice to see some tests of failure cases and malformed messages -- that may come later, but might be a good thing to do now. Maybe this can be tied in with custom generators for property-based testing. Let's talk about this too.


geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetAllRequestOperationHandler.java
Lines 59 (patched)
<https://reviews.apache.org/r/60718/#comment255524>

    It looks like this will throw `ClassCastException` if the key type doesn't satisfy key constraints. `LocalRegion`'s implementation calls `basicGetAll` to `accessEntry` to `validateKey`.
    
    I also followed `DataSetRegion`'s implementation through the code to 
    ```
    LocalRegion.get(Object key, Object aCallbackArgument, boolean generateCallbacks,
          boolean disableCopyOnRead, boolean preferCD, ClientProxyMembershipID requestingClient,
          EntryEventImpl clientEvent, boolean returnTombstones, boolean opScopeIsLocal,
          boolean retainResult) throws TimeoutException, CacheLoaderException {
    ```
    which calls `validateKey`, which can throw a `ClassCastException`.



geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandler.java
Lines 38 (patched)
<https://reviews.apache.org/r/60718/#comment255530>

    I had thought of operation handlers as being stateless. Is this not the case?



geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandler.java
Lines 59 (patched)
<https://reviews.apache.org/r/60718/#comment255529>

    This looks wrong. Shouldn't it be putting the keys somewhere?



geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufResponseUtilities.java
Line 40 (original), 38 (patched)
<https://reviews.apache.org/r/60718/#comment255531>

    Looks like we're creating error messages in this changeset, but not actually implementing the error message codes?



geode-protobuf/src/test/java/org/apache/geode/protocol/RoundTripCacheConnectionJUnitTest.java
Lines 155 (patched)
<https://reviews.apache.org/r/60718/#comment255532>

    I wonder if it would make sense to extracted this block to a method in a `ProtobufTestUtilities` class, or if that's overkill.
    
    In general, these tests look really noisy to me, but I'm not sure what the best way to improve that would be.



geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/GetAllRequestOperationHandlerJUnitTest.java
Lines 57 (patched)
<https://reviews.apache.org/r/60718/#comment255536>

    It seems to me that you're going to a lot of work to make the mock here. Would a spy be easier to understand?



geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandlerJUnitTest.java
Lines 68 (patched)
<https://reviews.apache.org/r/60718/#comment255537>

    The serialization service is stateless. Would a spy make more sense here?


- Galen O'Sullivan


On July 12, 2017, 6:27 p.m., Brian Rowe wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60718/
> -----------------------------------------------------------
> 
> (Updated July 12, 2017, 6:27 p.m.)
> 
> 
> Review request for geode, Alexander Murmann, Bruce Schuchardt, Galen O'Sullivan, Hitesh Khamesra, and Udo Kohlmeyer.
> 
> 
> Bugs: GEODE-2997
>     https://issues.apache.org/jira/browse/GEODE-2997
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> Changed get response to indicate if LookupFailure was a missing key or key with null value, added test
> Added GetAllRequestOperationHandler and unit test
> Added PutAllRequestOperationHandler and unit test
> Added an integration test covering the putAll and getAll operations
> 
> 
> Diffs
> -----
> 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufStreamProcessor.java 714639274 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetAllRequestOperationHandler.java PRE-CREATION 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetRequestOperationHandler.java 13b156f99 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandler.java PRE-CREATION 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/PutRequestOperationHandler.java fecf01d7b 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/RemoveRequestOperationHandler.java e1fef85b4 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufRequestUtilities.java b246a501b 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufResponseUtilities.java d6ef2788e 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufUtilities.java 924979329 
>   geode-protobuf/src/main/proto/clientProtocol.proto d94c0f312 
>   geode-protobuf/src/main/proto/region_API.proto 3108cb7c3 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/MessageUtil.java fee9448af 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/RoundTripCacheConnectionJUnitTest.java 612b9c9a4 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/GetAllRequestOperationHandlerJUnitTest.java PRE-CREATION 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/GetRequestOperationHandlerJUnitTest.java b7d52019e 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandlerJUnitTest.java PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60718/diff/4/
> 
> 
> Testing
> -------
> 
> Added unit tests for new operation handlers
> Added integration test covering new operations
> 
> 
> Thanks,
> 
> Brian Rowe
> 
>

Re: Review Request 60718: GEODE-2997: New flow getAll/putAll

Posted by Bruce Schuchardt <bs...@pivotal.io>.

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


Ship it!




Ship It!

- Bruce Schuchardt


On July 12, 2017, 11:27 a.m., Brian Rowe wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60718/
> -----------------------------------------------------------
> 
> (Updated July 12, 2017, 11:27 a.m.)
> 
> 
> Review request for geode, Alexander Murmann, Bruce Schuchardt, Galen O'Sullivan, Hitesh Khamesra, and Udo Kohlmeyer.
> 
> 
> Bugs: GEODE-2997
>     https://issues.apache.org/jira/browse/GEODE-2997
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> Changed get response to indicate if LookupFailure was a missing key or key with null value, added test
> Added GetAllRequestOperationHandler and unit test
> Added PutAllRequestOperationHandler and unit test
> Added an integration test covering the putAll and getAll operations
> 
> 
> Diffs
> -----
> 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufStreamProcessor.java 714639274 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetAllRequestOperationHandler.java PRE-CREATION 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetRequestOperationHandler.java 13b156f99 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandler.java PRE-CREATION 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/PutRequestOperationHandler.java fecf01d7b 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/RemoveRequestOperationHandler.java e1fef85b4 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufRequestUtilities.java b246a501b 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufResponseUtilities.java d6ef2788e 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufUtilities.java 924979329 
>   geode-protobuf/src/main/proto/clientProtocol.proto d94c0f312 
>   geode-protobuf/src/main/proto/region_API.proto 3108cb7c3 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/MessageUtil.java fee9448af 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/RoundTripCacheConnectionJUnitTest.java 612b9c9a4 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/GetAllRequestOperationHandlerJUnitTest.java PRE-CREATION 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/GetRequestOperationHandlerJUnitTest.java b7d52019e 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandlerJUnitTest.java PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60718/diff/4/
> 
> 
> Testing
> -------
> 
> Added unit tests for new operation handlers
> Added integration test covering new operations
> 
> 
> Thanks,
> 
> Brian Rowe
> 
>

Re: Review Request 60718: GEODE-2997: New flow getAll/putAll

Posted by Alexander Murmann <am...@pivotal.io>.

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


Ship it!




Ship It!

- Alexander Murmann


On July 12, 2017, 6:27 p.m., Brian Rowe wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60718/
> -----------------------------------------------------------
> 
> (Updated July 12, 2017, 6:27 p.m.)
> 
> 
> Review request for geode, Alexander Murmann, Bruce Schuchardt, Galen O'Sullivan, Hitesh Khamesra, and Udo Kohlmeyer.
> 
> 
> Bugs: GEODE-2997
>     https://issues.apache.org/jira/browse/GEODE-2997
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> Changed get response to indicate if LookupFailure was a missing key or key with null value, added test
> Added GetAllRequestOperationHandler and unit test
> Added PutAllRequestOperationHandler and unit test
> Added an integration test covering the putAll and getAll operations
> 
> 
> Diffs
> -----
> 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufStreamProcessor.java 714639274 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetAllRequestOperationHandler.java PRE-CREATION 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetRequestOperationHandler.java 13b156f99 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandler.java PRE-CREATION 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/PutRequestOperationHandler.java fecf01d7b 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/RemoveRequestOperationHandler.java e1fef85b4 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufRequestUtilities.java b246a501b 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufResponseUtilities.java d6ef2788e 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufUtilities.java 924979329 
>   geode-protobuf/src/main/proto/clientProtocol.proto d94c0f312 
>   geode-protobuf/src/main/proto/region_API.proto 3108cb7c3 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/MessageUtil.java fee9448af 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/RoundTripCacheConnectionJUnitTest.java 612b9c9a4 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/GetAllRequestOperationHandlerJUnitTest.java PRE-CREATION 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/GetRequestOperationHandlerJUnitTest.java b7d52019e 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandlerJUnitTest.java PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60718/diff/4/
> 
> 
> Testing
> -------
> 
> Added unit tests for new operation handlers
> Added integration test covering new operations
> 
> 
> Thanks,
> 
> Brian Rowe
> 
>

Re: Review Request 60718: GEODE-2997: New flow getAll/putAll

Posted by Brian Rowe <br...@pivotal.io>.

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

(Updated July 12, 2017, 6:27 p.m.)


Review request for geode, Alexander Murmann, Bruce Schuchardt, Galen O'Sullivan, Hitesh Khamesra, and Udo Kohlmeyer.


Changes
-------

Incorporated all feedback and updated ErrorResponse.


Bugs: GEODE-2997
    https://issues.apache.org/jira/browse/GEODE-2997


Repository: geode


Description
-------

Changed get response to indicate if LookupFailure was a missing key or key with null value, added test
Added GetAllRequestOperationHandler and unit test
Added PutAllRequestOperationHandler and unit test
Added an integration test covering the putAll and getAll operations


Diffs (updated)
-----

  geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufStreamProcessor.java 714639274 
  geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetAllRequestOperationHandler.java PRE-CREATION 
  geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetRequestOperationHandler.java 13b156f99 
  geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandler.java PRE-CREATION 
  geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/PutRequestOperationHandler.java fecf01d7b 
  geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/RemoveRequestOperationHandler.java e1fef85b4 
  geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufRequestUtilities.java b246a501b 
  geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufResponseUtilities.java d6ef2788e 
  geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufUtilities.java 924979329 
  geode-protobuf/src/main/proto/clientProtocol.proto d94c0f312 
  geode-protobuf/src/main/proto/region_API.proto 3108cb7c3 
  geode-protobuf/src/test/java/org/apache/geode/protocol/MessageUtil.java fee9448af 
  geode-protobuf/src/test/java/org/apache/geode/protocol/RoundTripCacheConnectionJUnitTest.java 612b9c9a4 
  geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/GetAllRequestOperationHandlerJUnitTest.java PRE-CREATION 
  geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/GetRequestOperationHandlerJUnitTest.java b7d52019e 
  geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandlerJUnitTest.java PRE-CREATION 


Diff: https://reviews.apache.org/r/60718/diff/4/

Changes: https://reviews.apache.org/r/60718/diff/3-4/


Testing
-------

Added unit tests for new operation handlers
Added integration test covering new operations


Thanks,

Brian Rowe

Re: Review Request 60718: GEODE-2997: New flow getAll/putAll

Posted by Brian Rowe <br...@pivotal.io>.

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

(Updated July 12, 2017, 6:23 p.m.)


Review request for geode, Alexander Murmann, Bruce Schuchardt, Galen O'Sullivan, Hitesh Khamesra, and Udo Kohlmeyer.


Bugs: GEODE-2997
    https://issues.apache.org/jira/browse/GEODE-2997


Repository: geode


Description
-------

Changed get response to indicate if LookupFailure was a missing key or key with null value, added test
Added GetAllRequestOperationHandler and unit test
Added PutAllRequestOperationHandler and unit test
Added an integration test covering the putAll and getAll operations


Diffs (updated)
-----

  geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufStreamProcessor.java 714639274 
  geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetAllRequestOperationHandler.java PRE-CREATION 
  geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandler.java PRE-CREATION 
  geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufRequestUtilities.java b246a501b 
  geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufResponseUtilities.java d6ef2788e 
  geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufUtilities.java 924979329 
  geode-protobuf/src/main/proto/region_API.proto 3108cb7c3 
  geode-protobuf/src/test/java/org/apache/geode/protocol/MessageUtil.java fee9448af 
  geode-protobuf/src/test/java/org/apache/geode/protocol/RoundTripCacheConnectionJUnitTest.java 612b9c9a4 
  geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/GetAllRequestOperationHandlerJUnitTest.java PRE-CREATION 
  geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/GetRequestOperationHandlerJUnitTest.java b7d52019e 
  geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandlerJUnitTest.java PRE-CREATION 


Diff: https://reviews.apache.org/r/60718/diff/3/

Changes: https://reviews.apache.org/r/60718/diff/2-3/


Testing
-------

Added unit tests for new operation handlers
Added integration test covering new operations


Thanks,

Brian Rowe

Re: Review Request 60718: GEODE-2997: New flow getAll/putAll

Posted by Brian Rowe <br...@pivotal.io>.

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

(Updated July 12, 2017, 6:01 p.m.)


Review request for geode, Alexander Murmann, Bruce Schuchardt, Galen O'Sullivan, Hitesh Khamesra, and Udo Kohlmeyer.


Bugs: GEODE-2997
    https://issues.apache.org/jira/browse/GEODE-2997


Repository: geode


Description
-------

Changed get response to indicate if LookupFailure was a missing key or key with null value, added test
Added GetAllRequestOperationHandler and unit test
Added PutAllRequestOperationHandler and unit test
Added an integration test covering the putAll and getAll operations


Diffs (updated)
-----

  geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufStreamProcessor.java 714639274 
  geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetAllRequestOperationHandler.java PRE-CREATION 
  geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandler.java PRE-CREATION 
  geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufRequestUtilities.java b246a501b 
  geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufResponseUtilities.java d6ef2788e 
  geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufUtilities.java 924979329 
  geode-protobuf/src/main/proto/region_API.proto 3108cb7c3 
  geode-protobuf/src/test/java/org/apache/geode/protocol/MessageUtil.java fee9448af 
  geode-protobuf/src/test/java/org/apache/geode/protocol/RoundTripCacheConnectionJUnitTest.java 612b9c9a4 
  geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/GetAllRequestOperationHandlerJUnitTest.java PRE-CREATION 
  geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/GetRequestOperationHandlerJUnitTest.java b7d52019e 
  geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandlerJUnitTest.java PRE-CREATION 


Diff: https://reviews.apache.org/r/60718/diff/2/

Changes: https://reviews.apache.org/r/60718/diff/1-2/


Testing
-------

Added unit tests for new operation handlers
Added integration test covering new operations


Thanks,

Brian Rowe

Re: Review Request 60718: GEODE-2997: New flow getAll/putAll

Posted by Brian Rowe <br...@pivotal.io>.


> On July 11, 2017, 5:17 p.m., Bruce Schuchardt wrote:
> > geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/GetRequestOperationHandlerJUnitTest.java
> > Lines 138 (patched)
> > <https://reviews.apache.org/r/60718/diff/1/?file=1771600#file1771600line138>
> >
> >     need to validate the response's value here

Good catch, fixed


- Brian


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


On July 12, 2017, 6:27 p.m., Brian Rowe wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60718/
> -----------------------------------------------------------
> 
> (Updated July 12, 2017, 6:27 p.m.)
> 
> 
> Review request for geode, Alexander Murmann, Bruce Schuchardt, Galen O'Sullivan, Hitesh Khamesra, and Udo Kohlmeyer.
> 
> 
> Bugs: GEODE-2997
>     https://issues.apache.org/jira/browse/GEODE-2997
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> Changed get response to indicate if LookupFailure was a missing key or key with null value, added test
> Added GetAllRequestOperationHandler and unit test
> Added PutAllRequestOperationHandler and unit test
> Added an integration test covering the putAll and getAll operations
> 
> 
> Diffs
> -----
> 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufStreamProcessor.java 714639274 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetAllRequestOperationHandler.java PRE-CREATION 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetRequestOperationHandler.java 13b156f99 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandler.java PRE-CREATION 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/PutRequestOperationHandler.java fecf01d7b 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/RemoveRequestOperationHandler.java e1fef85b4 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufRequestUtilities.java b246a501b 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufResponseUtilities.java d6ef2788e 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufUtilities.java 924979329 
>   geode-protobuf/src/main/proto/clientProtocol.proto d94c0f312 
>   geode-protobuf/src/main/proto/region_API.proto 3108cb7c3 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/MessageUtil.java fee9448af 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/RoundTripCacheConnectionJUnitTest.java 612b9c9a4 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/GetAllRequestOperationHandlerJUnitTest.java PRE-CREATION 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/GetRequestOperationHandlerJUnitTest.java b7d52019e 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandlerJUnitTest.java PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60718/diff/4/
> 
> 
> Testing
> -------
> 
> Added unit tests for new operation handlers
> Added integration test covering new operations
> 
> 
> Thanks,
> 
> Brian Rowe
> 
>

Re: Review Request 60718: GEODE-2997: New flow getAll/putAll

Posted by Bruce Schuchardt <bs...@pivotal.io>.

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




geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/GetRequestOperationHandlerJUnitTest.java
Lines 138 (patched)
<https://reviews.apache.org/r/60718/#comment255262>

    need to validate the response's value here


- Bruce Schuchardt


On July 7, 2017, 2:18 p.m., Brian Rowe wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60718/
> -----------------------------------------------------------
> 
> (Updated July 7, 2017, 2:18 p.m.)
> 
> 
> Review request for geode, Alexander Murmann, Bruce Schuchardt, Galen O'Sullivan, Hitesh Khamesra, and Udo Kohlmeyer.
> 
> 
> Bugs: GEODE-2997
>     https://issues.apache.org/jira/browse/GEODE-2997
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> Changed get response to indicate if LookupFailure was a missing key or key with null value, added test
> Added GetAllRequestOperationHandler and unit test
> Added PutAllRequestOperationHandler and unit test
> Added an integration test covering the putAll and getAll operations
> 
> 
> Diffs
> -----
> 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufStreamProcessor.java ebd5c6a0a 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetAllRequestOperationHandler.java PRE-CREATION 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandler.java PRE-CREATION 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufRequestUtilities.java b96f478d1 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufResponseUtilities.java 2114fdbf7 
>   geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufUtilities.java 924979329 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/RoundTripCacheConnectionJUnitTest.java 31a873658 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/GetAllRequestOperationHandlerJUnitTest.java PRE-CREATION 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/GetRequestOperationHandlerJUnitTest.java b7d52019e 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandlerJUnitTest.java PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60718/diff/1/
> 
> 
> Testing
> -------
> 
> Added unit tests for new operation handlers
> Added integration test covering new operations
> 
> 
> Thanks,
> 
> Brian Rowe
> 
>