Geode 1.8.0 maven repository is missing sources and javadoc jars

[Brian Rowe]

The maven repository for 1.8.0 seems to be missing the 1.8.0 source jars.
This means people using an IDE can't download these jars automatically to
see the geode source in their IDE.

Specifically, we see the following in 1.8 vs 1.7:
https://repo.maven.apache.org/maven2/org/apache/geode/geode-core/1.8.0/

geode-core-1.8.0.jar

 2018-11-30 03:23  12673898
geode-core-1.8.0.jar.asc

 2018-11-30 03:23   821
geode-core-1.8.0.jar.asc.md5

 2018-11-30 03:2332
geode-core-1.8.0.jar.asc.sha1

2018-11-30 03:2340
geode-core-1.8.0.jar.md5

 2018-11-30 03:2332
geode-core-1.8.0.jar.sha1

2018-11-30 03:2340
geode-core-1.8.0.pom

 2018-11-30 03:23 10855
geode-core-1.8.0.pom.asc

 2018-11-30 03:23   821
geode-core-1.8.0.pom.asc.md5

 2018-11-30 03:2332
geode-core-1.8.0.pom.asc.sha1

2018-11-30 03:2340
geode-core-1.8.0.pom.md5

 2018-11-30 03:2332
geode-core-1.8.0.pom.sha1

2018-11-30 03:2340



https://repo.maven.apache.org/maven2/org/apache/geode/geode-core/1.7.0/

geode-core-1.7.0-javadoc.jar

 2018-09-28 00:35  24484055
geode-core-1.7.0-javadoc.jar.asc

 2018-09-28 00:35   821
geode-core-1.7.0-javadoc.jar.asc.md5

 2018-09-28 00:3532
geode-core-1.7.0-javadoc.jar.asc.sha1

2018-09-28 00:3540
geode-core-1.7.0-javadoc.jar.md5

 2018-09-28 00:3532
geode-core-1.7.0-javadoc.jar.sha1

2018-09-28 00:3540
geode-core-1.7.0-sources.jar

 2018-09-28 00:35   9761555
geode-core-1.7.0-sources.jar.asc

 2018-09-28 00:35   821
geode-core-1.7.0-sources.jar.asc.md5

 2018-09-28 00:3532
geode-core-1.7.0-sources.jar.asc.sha1

2018-09-28 00:3540
geode-core-1.7.0-sources.jar.md5

 2018-09-28 00:3532
geode-core-1.7.0-sources.jar.sha1

2018-09-28 00:3540  geode-core-1.7.0.jar

 2018-09-28 00:35  13009887
geode-core-1.7.0.jar.asc

Re: proposing reduced default for "membership-port-range"

[Brian Rowe]

It should be, but I don't believe it can be.  Some of the places that we
pass the membership range to are third party.

On Thu, Oct 11, 2018 at 11:54 AM, Helena Bales  wrote:

> Could and should the port selecting logic be pulled out into one common
> implementation used throughout the code base? It seems like having a common
> implementation of this would make it a lot easier to solve this type of
> problem in the future.
>
> On Thu, Oct 11, 2018 at 11:27 AM Brian Rowe  wrote:
>
> > Galen, this was actually my first instinct.  Unfortunately this range is
> > passed into numerous places such as JGroups and BeanUtils, all of which
> > implement their own port selecting logic.
> >
> > On Thu, Oct 11, 2018 at 10:35 AM, Galen O'Sullivan <
> gosulli...@pivotal.io>
> > wrote:
> >
> > > Would it be feasible to reserve chosen ports before selecting ephemeral
> > > ports? I think this would resolve the collision issue described above.
> > >
> > > On Thu, Oct 11, 2018 at 9:58 AM Brian Rowe  wrote:
> > >
> > > > I agree that we should have defaulted everything to using ephemeral
> > ports
> > > > and forced clients to explicitly assign ports and membership ranges
> if
> > > > needed (any of the reasons Anthony mentioned above).  However, I
> don't
> > > > think we can change the default assigned ports at this point, unless
> we
> > > > want to try to go through the effort of verifying that no customers
> are
> > > > relying on the default assignments (is that even something we can do
> > with
> > > > any confidence?).  The membership port range default, however, is
> > > something
> > > > we should be able to change, so long as we restrict it to a proper
> > subset
> > > > of the current default.  I think the best we can do right now is just
> > to
> > > > make it so the default membership port range doesn't conflict with
> the
> > > > default assigned ports.
> > > >
> > > > On Fri, Oct 5, 2018 at 3:42 PM, Jacob Barrett 
> > > wrote:
> > > >
> > > > > So in the Dockerfile you explicitly set the server to start on port
> > > > 40404,
> > > > > problem solved. In whatever environment where you need it on a
> > specific
> > > > > port you then assign that port. But for all the other cases where
> we
> > > > don’t
> > > > > need to know it, like most of the time, it should just pick
> something
> > > > > ephemeral and work.
> > > > >
> > > > >
> > > > > > On Oct 5, 2018, at 1:57 PM, Anthony Baker 
> > wrote:
> > > > > >
> > > > > > I think there are a lot of dependencies when deploying geode that
> > > rely
> > > > > on well-known ports and port ranges (e.g. exporting ports from a
> > > > container,
> > > > > firewall rules, etc).  Changing the default server port from 40404
> to
> > > ??
> > > > > would break stuff.
> > > > > >
> > > > > > Here’s the rule from our own Dockerfile:
> > > > > >
> > > > > > # Default ports:
> > > > > > # RMI/JMX 1099
> > > > > > # REST 8080
> > > > > > # PULE 7070
> > > > > > # LOCATOR 10334
> > > > > > # CACHESERVER 40404
> > > > > > EXPOSE  8080 10334 40404 1099 7070
> > > > > >
> > > > > > Anthony
> > > > > >
> > > > > >
> > > > > >> On Oct 5, 2018, at 1:45 PM, Jacob Barrett 
> > > > wrote:
> > > > > >>
> > > > > >> But if all ports where ephemeral by default then no collisions
> > > right?
> > > > > Why have any port have a default to a single fixed value or
> > overlapping
> > > > > range of values. Since our opinionated use case is for clients to
> > > connect
> > > > > via locators then a known server port isn’t important.
> > > > > >>
> > > > > >>> On Oct 5, 2018, at 10:55 AM, Dan Smith 
> > wrote:
> > > > > >>>
> > > > > >>> The problem is that the membership port is picked *first*. So
> it
> > > may
> > > > > pick
> > > > > >>> 40404. Then, when the cache server tries to use port 40404, it
> > > gets 

Re: proposing reduced default for "membership-port-range"

[Brian Rowe]

Galen, this was actually my first instinct.  Unfortunately this range is
passed into numerous places such as JGroups and BeanUtils, all of which
implement their own port selecting logic.

On Thu, Oct 11, 2018 at 10:35 AM, Galen O'Sullivan 
wrote:

> Would it be feasible to reserve chosen ports before selecting ephemeral
> ports? I think this would resolve the collision issue described above.
>
> On Thu, Oct 11, 2018 at 9:58 AM Brian Rowe  wrote:
>
> > I agree that we should have defaulted everything to using ephemeral ports
> > and forced clients to explicitly assign ports and membership ranges if
> > needed (any of the reasons Anthony mentioned above).  However, I don't
> > think we can change the default assigned ports at this point, unless we
> > want to try to go through the effort of verifying that no customers are
> > relying on the default assignments (is that even something we can do with
> > any confidence?).  The membership port range default, however, is
> something
> > we should be able to change, so long as we restrict it to a proper subset
> > of the current default.  I think the best we can do right now is just to
> > make it so the default membership port range doesn't conflict with the
> > default assigned ports.
> >
> > On Fri, Oct 5, 2018 at 3:42 PM, Jacob Barrett 
> wrote:
> >
> > > So in the Dockerfile you explicitly set the server to start on port
> > 40404,
> > > problem solved. In whatever environment where you need it on a specific
> > > port you then assign that port. But for all the other cases where we
> > don’t
> > > need to know it, like most of the time, it should just pick something
> > > ephemeral and work.
> > >
> > >
> > > > On Oct 5, 2018, at 1:57 PM, Anthony Baker  wrote:
> > > >
> > > > I think there are a lot of dependencies when deploying geode that
> rely
> > > on well-known ports and port ranges (e.g. exporting ports from a
> > container,
> > > firewall rules, etc).  Changing the default server port from 40404 to
> ??
> > > would break stuff.
> > > >
> > > > Here’s the rule from our own Dockerfile:
> > > >
> > > > # Default ports:
> > > > # RMI/JMX 1099
> > > > # REST 8080
> > > > # PULE 7070
> > > > # LOCATOR 10334
> > > > # CACHESERVER 40404
> > > > EXPOSE  8080 10334 40404 1099 7070
> > > >
> > > > Anthony
> > > >
> > > >
> > > >> On Oct 5, 2018, at 1:45 PM, Jacob Barrett 
> > wrote:
> > > >>
> > > >> But if all ports where ephemeral by default then no collisions
> right?
> > > Why have any port have a default to a single fixed value or overlapping
> > > range of values. Since our opinionated use case is for clients to
> connect
> > > via locators then a known server port isn’t important.
> > > >>
> > > >>> On Oct 5, 2018, at 10:55 AM, Dan Smith  wrote:
> > > >>>
> > > >>> The problem is that the membership port is picked *first*. So it
> may
> > > pick
> > > >>> 40404. Then, when the cache server tries to use port 40404, it
> gets a
> > > >>> collision.
> > > >>>
> > > >>> -Dan
> > > >>>
> > > >>>> On Fri, Oct 5, 2018 at 10:52 AM Jacob Barrett <
> jbarr...@pivotal.io>
> > > wrote:
> > > >>>>
> > > >>>> If we just default to 0 then the OS will pick is a port in
> whatever
> > > range
> > > >>>> is ephemeral and free. We don’t have to do any work. No need to
> > > define a
> > > >>>> range and seek an open port.
> > > >>>>
> > > >>>>>> On Oct 5, 2018, at 10:40 AM, Dan Smith 
> wrote:
> > > >>>>>>
> > > >>>>>> On Fri, Oct 5, 2018 at 10:31 AM Jacob Barrett <
> > jbarr...@pivotal.io>
> > > >>>> wrote:
> > > >>>>>>
> > > >>>>>> Why not change the default behavior to that of port 0, letting
> the
> > > OS
> > > >>>>>> select an open ephemeral port if the user doesn’t specify a
> > specific
> > > >>>> port?
> > > >>>>>>
> > > >>>>>
> > > >>>>> I think what we'd really like to do is change the cache server
> port
> > > to
> > > >>>>> something other than 40404. Maybe 0 (pick a port), or maybe
> > something
> > > >>>> less
> > > >>>>> than 32K.
> > > >>>>>
> > > >>>>> Unfortunately, on most linux distributions the ephemeral port
> range
> > > is
> > > >>>> 32K
> > > >>>>> -> 61K, which includes 40404, which I think is why Brian is
> > > proposing a
> > > >>>>> subset of that range.
> > > >>>>>
> > > >>>>> -Dan
> > > >>>>
> > > >
> > >
> >
>

Re: proposing reduced default for "membership-port-range"

[Brian Rowe]

I agree that we should have defaulted everything to using ephemeral ports
and forced clients to explicitly assign ports and membership ranges if
needed (any of the reasons Anthony mentioned above).  However, I don't
think we can change the default assigned ports at this point, unless we
want to try to go through the effort of verifying that no customers are
relying on the default assignments (is that even something we can do with
any confidence?).  The membership port range default, however, is something
we should be able to change, so long as we restrict it to a proper subset
of the current default.  I think the best we can do right now is just to
make it so the default membership port range doesn't conflict with the
default assigned ports.

On Fri, Oct 5, 2018 at 3:42 PM, Jacob Barrett  wrote:

> So in the Dockerfile you explicitly set the server to start on port 40404,
> problem solved. In whatever environment where you need it on a specific
> port you then assign that port. But for all the other cases where we don’t
> need to know it, like most of the time, it should just pick something
> ephemeral and work.
>
>
> > On Oct 5, 2018, at 1:57 PM, Anthony Baker  wrote:
> >
> > I think there are a lot of dependencies when deploying geode that rely
> on well-known ports and port ranges (e.g. exporting ports from a container,
> firewall rules, etc).  Changing the default server port from 40404 to ??
> would break stuff.
> >
> > Here’s the rule from our own Dockerfile:
> >
> > # Default ports:
> > # RMI/JMX 1099
> > # REST 8080
> > # PULE 7070
> > # LOCATOR 10334
> > # CACHESERVER 40404
> > EXPOSE  8080 10334 40404 1099 7070
> >
> > Anthony
> >
> >
> >> On Oct 5, 2018, at 1:45 PM, Jacob Barrett  wrote:
> >>
> >> But if all ports where ephemeral by default then no collisions right?
> Why have any port have a default to a single fixed value or overlapping
> range of values. Since our opinionated use case is for clients to connect
> via locators then a known server port isn’t important.
> >>
> >>> On Oct 5, 2018, at 10:55 AM, Dan Smith  wrote:
> >>>
> >>> The problem is that the membership port is picked *first*. So it may
> pick
> >>> 40404. Then, when the cache server tries to use port 40404, it gets a
> >>> collision.
> >>>
> >>> -Dan
> >>>
>  On Fri, Oct 5, 2018 at 10:52 AM Jacob Barrett 
> wrote:
> 
>  If we just default to 0 then the OS will pick is a port in whatever
> range
>  is ephemeral and free. We don’t have to do any work. No need to
> define a
>  range and seek an open port.
> 
> >> On Oct 5, 2018, at 10:40 AM, Dan Smith  wrote:
> >>
> >> On Fri, Oct 5, 2018 at 10:31 AM Jacob Barrett 
>  wrote:
> >>
> >> Why not change the default behavior to that of port 0, letting the
> OS
> >> select an open ephemeral port if the user doesn’t specify a specific
>  port?
> >>
> >
> > I think what we'd really like to do is change the cache server port
> to
> > something other than 40404. Maybe 0 (pick a port), or maybe something
>  less
> > than 32K.
> >
> > Unfortunately, on most linux distributions the ephemeral port range
> is
>  32K
> > -> 61K, which includes 40404, which I think is why Brian is
> proposing a
> > subset of that range.
> >
> > -Dan
> 
> >
>

proposing reduced default for "membership-port-range"

[Brian Rowe]

Currently the default value for this parameter covers the default locator
and server port values.  As a result of this, when launching a locator and
then a server on the same system using gfsh, it's possible to see the
server fail because the locator has already bound the default server
socket.  We've actually seen a couple of test runs fail with this issue.

The proposed new range is 41000-61000, which, in addition to not
overlapping the other default port values, has the benefit of being a
subset of the linux ephemeral ports (for users who care about such
things).  Please let me know if there are any objections to this change.

Here's the current javadoc for this parameter:

Description: The allowed range of ports for use in forming an unique
membership identifier (UDP), for failure detection purposes (TCP) and to
listen on for peer connections (TCP). This range is given as two numbers
separated by a minus sign. Minimum 3 values in range are required to
successfully startup.
Default: 1024-65535

Re: Review Request 61816: GEODE-3409 Protobuf Client Can't Connect Once Connection Limit Has Been Reached, Even After Connections Closed

[Brian Rowe]

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


Ship it!




Ship It!

- Brian Rowe


On Aug. 23, 2017, 3:47 p.m., Bruce Schuchardt wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61816/
> ---
> 
> (Updated Aug. 23, 2017, 3:47 p.m.)
> 
> 
> Review request for geode, Alexander Murmann, Galen O'Sullivan, Hitesh 
> Khamesra, Udo Kohlmeyer, and Brian Rowe.
> 
> 
> Bugs: GEODE-3409
> https://issues.apache.org/jira/browse/GEODE-3409
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> ServerConnection cleanup was not decrementing the Acceptor's client 
> connection count when the protobuf communications mode was in effect.
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ClientHealthMonitor.java
>  35cc33f3fa5547a01368a3ae3b6ad401b858610d 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ServerConnection.java
>  394d2614c0d04353c95454c8730e84688e4766fe 
>   
> geode-protobuf/src/test/java/org/apache/geode/protocol/RoundTripCacheConnectionJUnitTest.java
>  12cc08b2f6cac77dcc2b79f1d3245f0960c3e8ba 
> 
> 
> Diff: https://reviews.apache.org/r/61816/diff/2/
> 
> 
> Testing
> ---
> 
> new integration test
> 
> 
> Thanks,
> 
> Bruce Schuchardt
> 
>

Re: Review Request 61672: GEODE-3249: internal messages should require credentials

[Brian Rowe]

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


Fix it, then Ship it!





geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ServerConnection.java
Lines 1059 (patched)
<https://reviews.apache.org/r/61672/#comment259090>

Is this intended to be new logging or was it just for debugging?



geode-core/src/test/java/org/apache/geode/security/ClientAuthenticationPart2DUnitTest.java
Lines 62 (patched)
<https://reviews.apache.org/r/61672/#comment259091>

Does this call do anything? serverConnection is a mock and you haven't told 
it how to handle this call.


- Brian Rowe


On Aug. 15, 2017, 8:46 p.m., Bruce Schuchardt wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61672/
> ---
> 
> (Updated Aug. 15, 2017, 8:46 p.m.)
> 
> 
> Review request for geode, Alexander Murmann, Galen O'Sullivan, Hitesh 
> Khamesra, Udo Kohlmeyer, and Brian Rowe.
> 
> 
> Bugs: GEODE-3249
> https://issues.apache.org/jira/browse/GEODE-3249
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> Internal messages that could alter server state now require security 
> credentials.
> 
> This was merely a matter of changing the server to require the credentials 
> and changing the client to send credentials.  I removed the general 
> overriding of AbstractOp.processSecureBytes() because it made no sense.  If 
> the server sends a secure byte "part" in a message the client is obligated to 
> process it or the next message it sends will cause a security violation.
> 
> I've added a server-side property that folks can set to allow old clients to 
> continue to work.  This must be used to roll the servers forward to the new 
> version that contains this change.  Clients must then be rolled forward & the 
> servers can then be rolled once again without the property set.
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/main/java/org/apache/geode/cache/client/internal/AbstractOp.java
>  c4035f9cf5db1c031e35eef4be0908afbddefffb 
>   
> geode-core/src/main/java/org/apache/geode/cache/client/internal/AddPDXEnumOp.java
>  ca7790aca5cab703c2180f85f01e37c91fa3c956 
>   
> geode-core/src/main/java/org/apache/geode/cache/client/internal/AddPDXTypeOp.java
>  88c85514c891d19399257bb2d85cb463b92ed6bb 
>   
> geode-core/src/main/java/org/apache/geode/cache/client/internal/CloseConnectionOp.java
>  ffcdc39c3ba05e90bf7b9c49509b72de70451f85 
>   
> geode-core/src/main/java/org/apache/geode/cache/client/internal/CommitOp.java 
> edffb2b18bde31435c9555b13c3e630aee1e4027 
>   
> geode-core/src/main/java/org/apache/geode/cache/client/internal/GetClientPRMetaDataOp.java
>  2ba3e3a9a8044fcd7d991fd444fcaf75b2a5c2f4 
>   
> geode-core/src/main/java/org/apache/geode/cache/client/internal/GetClientPartitionAttributesOp.java
>  49567dd31d9f617162768b5066bbb5307785a85f 
>   
> geode-core/src/main/java/org/apache/geode/cache/client/internal/GetEventValueOp.java
>  3fb5fcfa497264d5e0a14d95ed0935f392216680 
>   
> geode-core/src/main/java/org/apache/geode/cache/client/internal/GetFunctionAttributeOp.java
>  c7edbfea719e75291287824c3654c0e7fac3e7bb 
>   
> geode-core/src/main/java/org/apache/geode/cache/client/internal/GetPDXEnumByIdOp.java
>  7bbf74056f6ecfb7efe27c575029281b98d01b47 
>   
> geode-core/src/main/java/org/apache/geode/cache/client/internal/GetPDXEnumsOp.java
>  be4c092298df497f6c145b26d8b87234d59c6be8 
>   
> geode-core/src/main/java/org/apache/geode/cache/client/internal/GetPDXIdForEnumOp.java
>  d87371c6778e9a9ea44c956dbef9e169338c7930 
>   
> geode-core/src/main/java/org/apache/geode/cache/client/internal/GetPDXIdForTypeOp.java
>  27f600e3e5e2803cfd2f1c312036b57f61a12751 
>   
> geode-core/src/main/java/org/apache/geode/cache/client/internal/GetPDXTypeByIdOp.java
>  bee50b5f02c2d891f8c450ce1dc799757a39453f 
>   
> geode-core/src/main/java/org/apache/geode/cache/client/internal/GetPDXTypesOp.java
>  5256924e94fd533dc27c8eb28073a4e68bd68174 
>   
> geode-core/src/main/java/org/apache/geode/cache/client/internal/MakePrimaryOp.java
>  e1d3d5030bb2b31f6471cfc14f147d7780357dc1 
>   geode-core/src/main/java/org/apache/geode/cache/client/internal/PingOp.java 
> 2e5254226c3ef461e93033bb623dfca31cdce1c5 
>   
> geode-core/src/main/java/org/apache/geode/cache/client/internal/PrimaryAckOp.java
>  e380e99e00815d3d56763d429dfc8ad51c3f4113 
>   geode-core/src/ma

Re: Review Request 61540: GEODE-3403: Modify rolling upgrade test configuration for 1.2.x release

[Brian Rowe]

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


Ship it!




Ship It!

- Brian Rowe


On Aug. 9, 2017, 8:45 p.m., Bruce Schuchardt wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61540/
> ---
> 
> (Updated Aug. 9, 2017, 8:45 p.m.)
> 
> 
> Review request for geode, Alexander Murmann, Galen O'Sullivan, Udo Kohlmeyer, 
> and Brian Rowe.
> 
> 
> Bugs: GEODE-3403
> https://issues.apache.org/jira/browse/GEODE-3403
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> Nabarun has already added a test120 source set in geode-old-versions.  This 
> commit will roll the version on develop to 1.3.0 so that it will have a 
> different version ordinal than test120.
> 
> 
> Diffs
> -
> 
>   geode-core/src/main/java/org/apache/geode/internal/Version.java 
> 39e3a3f18d90567a1e3564884760014f6daf1f4c 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/CommandInitializer.java
>  9995e4658e5afcb5eb9823913e73aa04d1cdbdbd 
> 
> 
> Diff: https://reviews.apache.org/r/61540/diff/1/
> 
> 
> Testing
> ---
> 
> precheckin
> 
> 
> Thanks,
> 
> Bruce Schuchardt
> 
>

Geode Module Configuration

[Brian Rowe]

Hi all,

We are interested in adding some configuration parameters for the new
(pre-alpha) client-server protocol that lives in geode-protobuf. We're
curious if there is a good way to keep configuration out of geode-core
while still allowing users to set it conveniently.

It looks like for geode-lucene, there is some per-region configuration that
gets added to cache.xml files. Is there any global configuration?

Thanks,
Brian Rowe and Galen O'Sullivan

Re: Review Request 61420: GEODE-3307 CI failure: Uncaught exception in thread Thread[Geode Membership View Creator

[Brian Rowe]

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


Ship it!




Ship It!

- Brian Rowe


On Aug. 3, 2017, 10:07 p.m., Hitesh Khamesra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61420/
> ---
> 
> (Updated Aug. 3, 2017, 10:07 p.m.)
> 
> 
> Review request for geode, Alexander Murmann, Bruce Schuchardt, Galen 
> O'Sullivan, Udo Kohlmeyer, and Brian Rowe.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> Now we catch DistributedSystemDisconnectedException in ViewCreatoe thread and 
> then mark shutdown true
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/main/java/org/apache/geode/distributed/internal/membership/gms/membership/GMSJoinLeave.java
>  40a4254 
> 
> 
> Diff: https://reviews.apache.org/r/61420/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Hitesh Khamesra
> 
>

Re: Review Request 61411: GEODE-3286 Failing to cleanup connections from ConnectionTable receiver table (corrected "stopped" check in previous fix)

[Brian Rowe]

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


Fix it, then Ship it!




Nitpicks below, but change looks good.


geode-core/src/test/java/org/apache/geode/internal/tcp/ConnectionTableTest.java
Line 36 (original), 40 (patched)
<https://reviews.apache.org/r/61411/#comment257999>

Maybe make this @Before instead of calling it at the start of every test.



geode-core/src/test/java/org/apache/geode/internal/tcp/ConnectionTableTest.java
Line 54 (original), 70 (patched)
<https://reviews.apache.org/r/61411/#comment258000>

Add a when(connection.isReceiverStopped()).thenReturn(false);


- Brian Rowe


On Aug. 3, 2017, 6:59 p.m., Hitesh Khamesra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61411/
> ---
> 
> (Updated Aug. 3, 2017, 6:59 p.m.)
> 
> 
> Review request for geode, Alexander Murmann, Bruce Schuchardt, Galen 
> O'Sullivan, Udo Kohlmeyer, and Brian Rowe.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> In previous fix, we were checking "if thread is stopped then add connection 
> to receiver list". Instead, it should be if thread is stopped then we should 
> not add connection to receiver list. As a fix, we add connection to reciver 
> list if connection is not closed or thread is not stopped.
> 
> Added unit for it.
> 
> 
> Diffs
> -
> 
>   geode-core/src/main/java/org/apache/geode/internal/tcp/Connection.java 
> c3ad596 
>   geode-core/src/main/java/org/apache/geode/internal/tcp/ConnectionTable.java 
> 69fb7a2 
>   
> geode-core/src/test/java/org/apache/geode/internal/tcp/ConnectionTableTest.java
>  312c64d 
> 
> 
> Diff: https://reviews.apache.org/r/61411/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Hitesh Khamesra
> 
>

JIRA permissions

[Brian Rowe]

Hello all,

I'm having some trouble assigning tickets to myself and closing them in
JIRA.  Can someone grant me the appropriate permissions.  My username is:
WireBaron

Thanks,
-Brian

Can someone grant me wiki edit privileges

[Brian Rowe]

Can someone with the relevant karma please grant me edit permissions on
cwiki.apache.org/confluence.  My username there is browe.

Thanks,
-Brian

Re: Review Request 60856: GEODE-3052 Need to reset isCoordinator flag in GMSLocator.

[Brian Rowe]

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


Ship it!




Ship It!

- Brian Rowe


On July 13, 2017, 11:28 p.m., Hitesh Khamesra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60856/
> ---
> 
> (Updated July 13, 2017, 11:28 p.m.)
> 
> 
> Review request for geode, Alexander Murmann, Bruce Schuchardt, Galen 
> O'Sullivan, and Udo Kohlmeyer.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> isCoordinator flag ensures that this process is becoming the
> coordinator thus other process should join this process. But
> when network parttion happens, we were not resetting this flag.
> 
> Now we reset isCoordinator flag when viewCreator thread shutdowns.
> 
> added unit test for it.
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/main/java/org/apache/geode/distributed/internal/membership/gms/locator/GMSLocator.java
>  2c56f5b 
>   
> geode-core/src/main/java/org/apache/geode/distributed/internal/membership/gms/membership/GMSJoinLeave.java
>  9591673 
>   
> geode-core/src/test/java/org/apache/geode/distributed/internal/membership/gms/membership/GMSJoinLeaveJUnitTest.java
>  d8c12e2 
> 
> 
> Diff: https://reviews.apache.org/r/60856/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Hitesh Khamesra
> 
>

Review Request 60834: GEODE-3051: Remove unreachable exception handling in AcceptorImpl.accept

[Brian Rowe]

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

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


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


Repository: geode


Description
---

This removes handling of SSL exceptions from the AccepterImpl.accept call, as 
the SSL handling code is now all done in another thread.
The exception handling being done in the other thread appears to be correct, as 
validated by CacheServerSSLConnectionDUnitTest.testNonSSLClient


Diffs
-

  
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/AcceptorImpl.java
 3c424d397 


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


Testing
---


Thanks,

Brian Rowe

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

[Brian Rowe]

> 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

[Brian Rowe]

> 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

[Brian Rowe]

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 <mst...@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 

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

[Brian Rowe]

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

[Brian Rowe]

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

[Brian Rowe]

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

[Brian Rowe]

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

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

[Brian Rowe]

> 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 60526: GEODE-3121: Verify SSL works with new protobuf protocol

[Brian Rowe]

> On July 7, 2017, 8:58 p.m., Udo Kohlmeyer wrote:
> > geode-core/src/main/java/org/apache/geode/internal/net/SocketCreator.java
> > Line 267 (original), 267 (patched)
> > <https://reviews.apache.org/r/60526/diff/2/?file=1771574#file1771574line267>
> >
> > You should maybe use the `SocketCreatorFactory`. We don't want to 
> > expose this class outside of the package.

Good call, this cleans this up nicely.


- Brian


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


On July 7, 2017, 10:16 p.m., Brian Rowe wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60526/
> ---
> 
> (Updated July 7, 2017, 10:16 p.m.)
> 
> 
> Review request for geode, Alexander Murmann, Bruce Schuchardt, Galen 
> O'Sullivan, Hitesh Khamesra, and Udo Kohlmeyer.
> 
> 
> Bugs: GEODE-3121
> https://issues.apache.org/jira/browse/GEODE-3121
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> Adding a new test to run the protobuf integration test over a SSL connection.
> 
> 
> Diffs
> -
> 
>   
> geode-protobuf/src/test/java/org/apache/geode/protocol/RoundTripCacheConnectionJUnitTest.java
>  31a873658 
>   
> geode-protobuf/src/test/resources/org/apache/geode/protocol/default.keystore 
> PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60526/diff/3/
> 
> 
> Testing
> ---
> 
> This change is just adding a test.
> 
> 
> Thanks,
> 
> Brian Rowe
> 
>

Re: Review Request 60526: GEODE-3121: Verify SSL works with new protobuf protocol

[Brian Rowe]

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

(Updated July 7, 2017, 10:16 p.m.)


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


Changes
---

Changing to use SocketCreatorFactory.


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


Repository: geode


Description
---

Adding a new test to run the protobuf integration test over a SSL connection.


Diffs (updated)
-

  
geode-protobuf/src/test/java/org/apache/geode/protocol/RoundTripCacheConnectionJUnitTest.java
 31a873658 
  geode-protobuf/src/test/resources/org/apache/geode/protocol/default.keystore 
PRE-CREATION 


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

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


Testing
---

This change is just adding a test.


Thanks,

Brian Rowe

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

[Brian Rowe]

---
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 60526: GEODE-3121: Verify SSL works with new protobuf protocol

[Brian Rowe]

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

(Updated July 7, 2017, 8:54 p.m.)


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


Changes
---

Updated to reflect changes to develop and uploading new diff.


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


Repository: geode


Description
---

Adding a new test to run the protobuf integration test over a SSL connection.


Diffs (updated)
-

  geode-core/src/main/java/org/apache/geode/internal/net/SocketCreator.java 
844b48413 
  
geode-protobuf/src/test/java/org/apache/geode/protocol/RoundTripCacheConnectionJUnitTest.java
 31a873658 
  geode-protobuf/src/test/resources/org/apache/geode/protocol/default.keystore 
PRE-CREATION 


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

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


Testing
---

This change is just adding a test.


Thanks,

Brian Rowe

Re: Review Request 60486: GEODE-3105: Adding handler for GetRegions

[Brian Rowe]

> On June 28, 2017, 3:53 a.m., Udo Kohlmeyer wrote:
> > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufOpsProcessor.java
> > Line 50 (original), 50 (patched)
> > <https://reviews.apache.org/r/60486/diff/1/?file=1764594#file1764594line50>
> >
> > Feels like this is going to end up being a TranslationUtilService. Can 
> > you discusss internally... `wrapResponseForOperationTypeID` is a candidate 
> > for movement as well...

These functions are no longer necessary and have been removed in the GEODE-3129 
change.


> On June 28, 2017, 3:53 a.m., Udo Kohlmeyer wrote:
> > geode-protobuf/src/main/proto/region_API.proto
> > Line 107 (original), 107 (patched)
> > <https://reviews.apache.org/r/60486/diff/1/?file=1764597#file1764597line107>
> >
> > What does this represent? `Failure` should be defined in the  `Success` 
> > can be defined as either an empty or populated regions list.
> > 
> > In the `ResponseHeader` does contain an `errorCode`. What we seem to be 
> > missing is a real `ErrorMessage`, that would contain the `exceptionCode` 
> > and `errorMessage`.

This has been removed.


- Brian


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


On June 27, 2017, 11:31 p.m., Brian Rowe wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60486/
> ---
> 
> (Updated June 27, 2017, 11:31 p.m.)
> 
> 
> Review request for geode, Alexander Murmann, Bruce Schuchardt, Galen 
> O'Sullivan, Hitesh Khamesra, and Udo Kohlmeyer.
> 
> 
> Bugs: GEODE-3105
> https://issues.apache.org/jira/browse/GEODE-3105
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> Added a handler which will catch incoming getRegion requests and will call 
> into the cache's rootRegion and return the names of the regions it finds.
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/GenericProtocolServerConnection.java
>  8edd83c0f 
>   
> geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufOpsProcessor.java
>  29d33170c 
>   
> geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufStreamProcessor.java
>  21dbef5bc 
>   
> geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetRegionsRequestOperationHandler.java
>  PRE-CREATION 
>   geode-protobuf/src/main/proto/region_API.proto adeb011c6 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/MessageUtil.java 
> 73d0803b6 
>   
> geode-protobuf/src/test/java/org/apache/geode/protocol/RoundTripCacheConnectionJUnitTest.java
>  1fbe82159 
>   
> geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/GetRegionRequestOperationHandlerJUnitTest.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60486/diff/1/
> 
> 
> Testing
> ---
> 
> Added unit test verifying hanlder behavior.
> Added integration test verifying module correctness for getRegion.
> Running precheckin.
> 
> 
> Thanks,
> 
> Brian Rowe
> 
>

Re: Review Request 60451: GEODE-2996: adding Put handler

[Brian Rowe]

> On June 28, 2017, 4:24 a.m., Udo Kohlmeyer wrote:
> > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufOpsProcessor.java
> > Lines 68 (patched)
> > <https://reviews.apache.org/r/60451/diff/1/?file=1763372#file1763372line70>
> >
> > This has nothing to do with the ProtobufOpsProcessor. Maybe the builder 
> > can be passed in.
> 
> Brian Rowe wrote:
> Hmm, I'm not sure I agree. The ProtobufOpsProcessor has to generate the 
> ClientProtocol.Response containing the response from the operation handler. 
> It seems reasonable for it to know how to build this object. Having the 
> builder needing to be passed into this seems similar to requiring this class 
> to pass the builder for the operation response down to the operation handler.
> 
> Udo Kohlmeyer wrote:
> I would try and contain all the builder code and Protobuf message 
> building stuff to the utility. This way we have a single place where we do 
> anything with the protobuf messages

In the change for GEODE-3129 all of the builder specific code got moved into 
utility objects.


> On June 28, 2017, 4:24 a.m., Udo Kohlmeyer wrote:
> > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetRequestOperationHandler.java
> > Lines 62 (patched)
> > <https://reviews.apache.org/r/60451/diff/1/?file=1763375#file1763375line66>
> >
> > Maybe this should return an empty list.
> 
> Brian Rowe wrote:
> I'm going to talk with the team tomorrow and see if we can't come up with 
> some way to indicate optional arguments.

Will return an empty EncodedValue in 3129 change.


> On June 28, 2017, 4:24 a.m., Udo Kohlmeyer wrote:
> > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetRequestOperationHandler.java
> > Lines 66 (patched)
> > <https://reviews.apache.org/r/60451/diff/1/?file=1763375#file1763375line70>
> >
> > This provides no information about the failure... An `ErrorMessage` 
> > should be a more suitable replacement
> 
> Brian Rowe wrote:
> Yes, once we have an error message we'll hopefully not even return a 
> GetResponse here (which will make things more complicated in the 
> OpsProcessor).
> 
> Udo Kohlmeyer wrote:
> I think it would be simple to have a "success" and a "failure" branch in 
> the code.. If there is a failure, we consistently create an `ErrorMessage`. 
> Don't think that logic would be too hard to create or even make it generic 
> for all operations.

Success is removed in 3129.


> On June 28, 2017, 4:24 a.m., Udo Kohlmeyer wrote:
> > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetRequestOperationHandler.java
> > Line 59 (original), 78 (patched)
> > <https://reviews.apache.org/r/60451/diff/1/?file=1763375#file1763375line82>
> >
> > Can I have a `success=false` and `keyExists=true`? What does 
> > `keyExists` provide me in functionality above and beyond `success` or an 
> > `ErrorMessage`?
> 
> Brian Rowe wrote:
> keyExists was supposed to differentiate between not present vs. nulled 
> keys. However in talking through this with Galen it's clear that we need to 
> think through this a bit more.

Removed in 3129 change.


> On June 28, 2017, 4:24 a.m., Udo Kohlmeyer wrote:
> > geode-protobuf/src/main/proto/region_API.proto
> > Line 35 (original), 35 (patched)
> > <https://reviews.apache.org/r/60451/diff/1/?file=1763380#file1763380line35>
> >
> > not sure `success` or `keyExists` provides any value here `result` 
> > should either have a value or not. 
> > What does `keyExists` denote? 
> > What counts as a `failure`?
> 
> Brian Rowe wrote:
> Unfortunately the protobuf protocol doesn't really give us a way to not 
> have a result. keyExists is supposed to denote whether the key was found 
> independant of whether or not it had a value, but we still need to figure out 
> how to encode a lack of value. A failure would be anything that kept us from 
> even running the query. An error decoding the key would be an example.

Added proper errorResponse in 3129.


> On June 28, 2017, 4:24 a.m., Udo Kohlmeyer wrote:
> > geode-protobuf/src/test/java/org/apache/geode/protocol/MessageUtil.java
> > Line 20 (original), 31 (patched)
> > <https://reviews.apache.org/r/60451/diff/1/?file=1763381#file1763381line31>
> >
> > Given this class is Protobuf specific, we either put it in the 
> > `protobuf` specific package or call it `ProtobufMessageUtil`
> 
> Brian Rowe wrote:
> I like ProtobufTestMessageUtil just to make it clear that it's both 
> protobuf and test sp

Re: Review Request 60629: GEODE-3129 - Added error messages to protobuf protocol

[Brian Rowe]

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

Re: Review Request 60629: GEODE-3129 - Added error messages to protobuf protocol

[Brian Rowe]

> On July 6, 2017, 8:45 p.m., Udo Kohlmeyer wrote:
> > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufStreamProcessor.java
> > Line 67 (original), 68 (patched)
> > <https://reviews.apache.org/r/60629/diff/1/?file=1768746#file1768746line72>
> >
> > Why do we have `processOneMessage`? How does this method ensure that 
> > only a single message is processed. It is misleading and makes no sense. 
> > Think we should stick to `processMessage`

This call specifically reads a single message from the incoming socket and 
writes a single message to the outgoing socket.  It's called processOneMessage 
to make it clear that it's not handling multiple messages.


> On July 6, 2017, 8:45 p.m., Udo Kohlmeyer wrote:
> > geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufRequestUtilities.java
> > Lines 57 (patched)
> > <https://reviews.apache.org/r/60629/diff/1/?file=1768752#file1768752line57>
> >
> > Misleading method name. It is not returning a `PutRequest` but rather a 
> > `Request` object.

That was intentional as the PutRequest is not generally useful outside of a 
Request object. Do you think there is value in a function that returns just a 
PutRequest, or do you just object to the name (what would you propose as an 
alternative?)?


> 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 101 (patched)
> > <https://reviews.apache.org/r/60629/diff/1/?file=1768753#file1768753line101>
> >
> > This is misleading. This is returning a `Response` object rather than a 
> > `GetRegionNamesResponse` object.

Same as two comments previous, we can discuss this futher in person.


> On July 6, 2017, 8:45 p.m., Udo Kohlmeyer wrote:
> > geode-protobuf/src/main/proto/clientProtocol.proto
> > Line 53 (original), 53 (patched)
> > <https://reviews.apache.org/r/60629/diff/1/?file=1768756#file1768756line53>
> >
> > How about `GetAvailableRegions`?

The main goal here was just to make it differentiated from GetRegion (which is 
apparently the API that PDD actually wants).  I do feel that getRegionNames is 
a bit more accurate since this returns just a list of the names of regions, 
rather than the entire region objects.


> On July 6, 2017, 8:45 p.m., Udo Kohlmeyer wrote:
> > geode-protobuf/src/main/proto/clientProtocol.proto
> > Lines 80 (patched)
> > <https://reviews.apache.org/r/60629/diff/1/?file=1768756#file1768756line80>
> >
> > Why does a client care if it is a server error or not? What does this 
> > mean for a client?

The error response fields were dictated by the PDD team, we should sit down 
with them to define what the specific meaning of the fields are (as I'm not 
even sure when I'm setting them).


> On July 6, 2017, 8:45 p.m., Udo Kohlmeyer wrote:
> > geode-protobuf/src/main/proto/clientProtocol.proto
> > Lines 81 (patched)
> > <https://reviews.apache.org/r/60629/diff/1/?file=1768756#file1768756line81>
> >
> > How does one determine if it is `retriable` or not? Surely every type 
> > of error should be seen as a failure rather than `try again later`

Same as previous response, we'll get this clarified by PDD.


> On July 6, 2017, 8:45 p.m., Udo Kohlmeyer wrote:
> > geode-protobuf/src/main/proto/region_API.proto
> > Lines 36-38 (original), 42-45 (patched)
> > <https://reviews.apache.org/r/60629/diff/1/?file=1768757#file1768757line42>
> >
> > This should really still only be an `EncodedValue`. Special casing the 
> > `GetResponse` payload should not be required.

Agreed, I didn't realize that we could just have a null EncodedValue.


> On July 6, 2017, 8:45 p.m., Udo Kohlmeyer wrote:
> > geode-protobuf/src/main/proto/region_API.proto
> > Line 37 (original), 43 (patched)
> > <https://reviews.apache.org/r/60629/diff/1/?file=1768757#file1768757line43>
> >
> > I don't think we need this. Too many conditional checks. Maybe we pass 
> > back a `null` for the `result`.

No, null EncodedValue is sufficient. Removed this field.


> On July 6, 2017, 8:45 p.m., Udo Kohlmeyer wrote:
> > geode-protobuf/src/main/proto/region_API.proto
> > Line 102 (original), 109 (patched)
> > <https://reviews.apache.org/r/60629/diff/1/?file=1768757#file1768757line109>
> >
> > As raised earlier, how about `GetAvailableRegions`?

See my response to that comment.


- Brian


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

Review Request 60629: GEODE-3129 - Added error messages to protobuf protocol

[Brian Rowe]

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

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

Re: Review Request 60550: GEODE-3154: add geode-protobuf to expected_jars.txt

[Brian Rowe]

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


Ship it!




Ship It!

- Brian Rowe


On June 29, 2017, 9:39 p.m., Galen O'Sullivan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60550/
> ---
> 
> (Updated June 29, 2017, 9:39 p.m.)
> 
> 
> Review request for geode, Alexander Murmann, Barry Oglesby, Bruce Schuchardt, 
> Hitesh Khamesra, Udo Kohlmeyer, and Brian Rowe.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> Currently BundledJarsJUnitTest is failing because we added geode-protobuf to 
> geode-assembly but not to the `expected_jars.txt` file, which tells us which 
> jars we expect. This one-liner adds it. This should keep builds from failing.
> 
> 
> Diffs
> -
> 
>   geode-assembly/src/test/resources/expected_jars.txt 62601677f 
> 
> 
> Diff: https://reviews.apache.org/r/60550/diff/1/
> 
> 
> Testing
> ---
> 
> Built on my machine, verified that the test passes.
> 
> 
> Thanks,
> 
> Galen O'Sullivan
> 
>

Re: Review Request 60451: GEODE-2996: adding Put handler

[Brian Rowe]

dant 
of whether or not it had a value, but we still need to figure out how to encode 
a lack of value. A failure would be anything that kept us from even running the 
query. An error decoding the key would be an example.


> On June 28, 2017, 4:24 a.m., Udo Kohlmeyer wrote:
> > geode-protobuf/src/test/java/org/apache/geode/protocol/MessageUtil.java
> > Line 20 (original), 31 (patched)
> > <https://reviews.apache.org/r/60451/diff/1/?file=1763381#file1763381line31>
> >
> > Given this class is Protobuf specific, we either put it in the 
> > `protobuf` specific package or call it `ProtobufMessageUtil`

I like ProtobufTestMessageUtil just to make it clear that it's both protobuf 
and test specific.


> On June 28, 2017, 4:24 a.m., Udo Kohlmeyer wrote:
> > geode-protobuf/src/test/java/org/apache/geode/protocol/MessageUtil.java
> > Lines 46 (patched)
> > <https://reviews.apache.org/r/60451/diff/1/?file=1763381#file1763381line46>
> >
> > This is too specific... What about all the other encoding types? 
> > Int,long,short,byte?

We discussed templatizing this to make it more generally useful, but decided 
that since this is a test utility it wasn't really worth making that change 
unless it was needed.


- Brian


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


On June 27, 2017, 1:20 a.m., Brian Rowe wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60451/
> ---
> 
> (Updated June 27, 2017, 1:20 a.m.)
> 
> 
> Review request for geode, Alexander Murmann, Bruce Schuchardt, Galen 
> O'Sullivan, Hitesh Khamesra, and Udo Kohlmeyer.
> 
> 
> Bugs: GEODE-2996
> https://issues.apache.org/jira/browse/GEODE-2996
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> This is a continuation of the review Alex submitted this morning with the 
> following changes:
> 
> Addresses review feedback for GEODE-2996, mainly refactoring 
> getOpertionHandler to handle failures like the putOperationHandler
> Adding put operations to the RoundTripCacheConnectionJUnitTest, which is the 
> integration test for the protobuf module
> Removing service loading for protobuf operations and instead have the 
> ProtobufStreamProcessor populate its OperationHandlerRegistry
> Remove exception throwing from OperationHandler.process calls and remove 
> TypeEncodingException
> Fixing ProtobufOpsProcessor to handle responses for types other than get
> 
> 
> Diffs
> -
> 
>   
> geode-protobuf/src/main/java/org/apache/geode/protocol/operations/OperationHandler.java
>  7683e3bf3 
>   
> geode-protobuf/src/main/java/org/apache/geode/protocol/operations/registry/OperationsHandlerRegistry.java
>  8e3a33149 
>   
> geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufOpsProcessor.java
>  d426149e4 
>   
> geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufStreamProcessor.java
>  d7b5d4bd2 
>   
> geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufUtilities.java
>  d76366298 
>   
> geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetRequestOperationHandler.java
>  d9c14752f 
>   
> geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/PutRequestOperationHandler.java
>  PRE-CREATION 
>   
> geode-protobuf/src/main/java/org/apache/geode/serialization/exception/TypeEncodingException.java
>  f3145a774 
>   
> geode-protobuf/src/main/java/org/apache/geode/serialization/exception/UnsupportedEncodingTypeException.java
>  c577e768a 
>   
> geode-protobuf/src/main/java/org/apache/geode/serialization/registry/exception/CodecNotRegisteredForTypeException.java
>  5c923a520 
>   geode-protobuf/src/main/proto/region_API.proto 52291c451 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/MessageUtil.java 
> f0b0b417b 
>   
> geode-protobuf/src/test/java/org/apache/geode/protocol/RoundTripCacheConnectionJUnitTest.java
>  b9faca3c9 
>   
> geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/ProtobufOpsProcessorJUnitTest.java
>  fc980aec9 
>   
> geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/GetRequestOperationHandlerJUnitTest.java
>  daa5870ed 
>   
> geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/PutRequestOperationHandlerJUnitTest.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60451/diff/1/
> 
> 
> Testing
> ---
> 
> Unit tests, whole module test, precheckin in progress.
> 
> 
> Thanks,
> 
> Brian Rowe
> 
>

Re: Review Request 60523: GEODE-3141 New flow: GetRegion

[Brian Rowe]

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




geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetRegionRequestOperationHandler.java
Lines 39 (patched)
<https://reviews.apache.org/r/60523/#comment253796>

If the region legitimately doesn't exist, is this really a failure?  Maybe 
we should rename the 'success' field to something like 'found'.



geode-protobuf/src/main/proto/region_API.proto
Lines 119 (patched)
<https://reviews.apache.org/r/60523/#comment253798>

I think it'd be cleaner to add these fields to the Region protobuf object 
(we'd also need to change the GetRegionsResponse to contain strings instead of 
Regions, which also seems like an improvement).


- Brian Rowe


On June 28, 2017, 11:06 p.m., Bruce Schuchardt wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60523/
> ---
> 
> (Updated June 28, 2017, 11:06 p.m.)
> 
> 
> Review request for geode, Alexander Murmann, Galen O'Sullivan, Hitesh 
> Khamesra, Udo Kohlmeyer, and Brian Rowe.
> 
> 
> Bugs: GEODE-3141
> https://issues.apache.org/jira/browse/GEODE-3141
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> Added a GetRegion(regionName) API.  More details about the server region can 
> be added if needed.  This implementation just has a success flag and flag 
> saying whether the region is destroyed.
> 
> 
> Diffs
> -
> 
>   
> geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufOpsProcessor.java
>  4318fb444dd0eb5f5d07175a466e26f03933cc4d 
>   
> geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufStreamProcessor.java
>  4e76de4a15d1ff0cfeddcc2c7115c7c18c2e14ba 
>   
> geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetRegionRequestOperationHandler.java
>  PRE-CREATION 
>   geode-protobuf/src/main/proto/clientProtocol.proto 
> 0c192950a4e750f7c55e186c18a79a6c52716bf0 
>   geode-protobuf/src/main/proto/region_API.proto 
> d3af17acb1ee45c134837c321b2009c8532817cc 
>   geode-protobuf/src/test/java/org/apache/geode/protocol/MessageUtil.java 
> dc897241fc229da53a5ff7a0e2df1678b8bceb6d 
>   
> geode-protobuf/src/test/java/org/apache/geode/protocol/RoundTripCacheConnectionJUnitTest.java
>  77b984f7e28a7bfa21b8e1c8c6a83467e9a37242 
>   
> geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/GetRegionRequestOperationHandlerJUnitTest.java
>  e8f1e651a71240474a793d005164df1ba5d4cda7 
>   
> geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/GetRegionsRequestOperationHandlerJUnitTest.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60523/diff/1/
> 
> 
> Testing
> ---
> 
> new unit tests
> 
> 
> Thanks,
> 
> Bruce Schuchardt
> 
>

Review Request 60526: GEODE-3121: Verify SSL works with new protobuf protocol

[Brian Rowe]

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

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


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


Repository: geode


Description
---

Adding a new test to run the protobuf integration test over a SSL connection.


Diffs
-

  geode-core/src/main/java/org/apache/geode/internal/net/SocketCreator.java 
844b48413 
  
geode-protobuf/src/test/java/org/apache/geode/protocol/RoundTripCacheConnectionJUnitTest.java
 77b984f7e 
  geode-protobuf/src/test/resources/org/apache/geode/protocol/default.keystore 
PRE-CREATION 


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


Testing
---

This change is just adding a test.


Thanks,

Brian Rowe

Review Request 60486: GEODE-3105: Adding handler for GetRegions

[Brian Rowe]

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

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


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


Repository: geode


Description
---

Added a handler which will catch incoming getRegion requests and will call into 
the cache's rootRegion and return the names of the regions it finds.


Diffs
-

  
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/GenericProtocolServerConnection.java
 8edd83c0f 
  
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufOpsProcessor.java
 29d33170c 
  
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufStreamProcessor.java
 21dbef5bc 
  
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetRegionsRequestOperationHandler.java
 PRE-CREATION 
  geode-protobuf/src/main/proto/region_API.proto adeb011c6 
  geode-protobuf/src/test/java/org/apache/geode/protocol/MessageUtil.java 
73d0803b6 
  
geode-protobuf/src/test/java/org/apache/geode/protocol/RoundTripCacheConnectionJUnitTest.java
 1fbe82159 
  
geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/GetRegionRequestOperationHandlerJUnitTest.java
 PRE-CREATION 


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


Testing
---

Added unit test verifying hanlder behavior.
Added integration test verifying module correctness for getRegion.
Running precheckin.


Thanks,

Brian Rowe

Re: Review Request 60312: GEODE-2804 Cache InetAddress if configure host as ip string.

[Brian Rowe]

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




geode-core/src/main/java/org/apache/geode/cache/client/internal/AutoConnectionSourceImpl.java
Line 86 (original), 89 (patched)
<https://reviews.apache.org/r/60312/#comment253371>

Since you're poking around in here anyways, can you clean up the formatting 
here



geode-core/src/main/java/org/apache/geode/cache/client/internal/AutoConnectionSourceImpl.java
Line 106 (original), 109 (patched)
<https://reviews.apache.org/r/60312/#comment253373>

This doesn't appear to be the most useful javadoc ever, but at the very 
least you should add the new parameter to it.



geode-core/src/main/java/org/apache/geode/cache/client/internal/AutoConnectionSourceImpl.java
Lines 117 (patched)
<https://reviews.apache.org/r/60312/#comment253375>

Did you mean to leave this comment in?



geode-core/src/main/java/org/apache/geode/distributed/internal/tcpserver/TcpClient.java
Line 161 (original), 161 (patched)
<https://reviews.apache.org/r/60312/#comment253391>

Looks like you wanted to ask Bruce about this, you probably shouldn't merge 
this comment.



geode-core/src/main/java/org/apache/geode/internal/admin/remote/DistributionLocatorId.java
Lines 235 (patched)
<https://reviews.apache.org/r/60312/#comment253396>

Can we remove all of the the this. calls in favor of just using 
 directly?  Failing that, can you change 225 and 226 to this.hostname 
for consistency.



geode-core/src/main/java/org/apache/geode/internal/cache/PoolFactoryImpl.java
Lines 59 (patched)
<https://reviews.apache.org/r/60312/#comment253397>

Typo, s/loators/locators



geode-core/src/main/java/org/apache/geode/internal/cache/PoolFactoryImpl.java
Lines 308 (patched)
<https://reviews.apache.org/r/60312/#comment253399>

remove this


- Brian Rowe


On June 21, 2017, 9:35 p.m., Hitesh Khamesra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60312/
> ---
> 
> (Updated June 21, 2017, 9:35 p.m.)
> 
> 
> Review request for geode, Alexander Murmann, Bruce Schuchardt, and Galen 
> O'Sullivan.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> GEODE-2804 Cache InetAddress if configure host as ip string.
> 
> 1. We keep configure host string in HostAddress class
> 2. We reuse InetsocketAddress if it is ipString.
> 3. Client has logic to retry thus we keep InetsocketAddress even if 
>it is not ip string.
> 
> GEODE-3017 IPv6 issue on windows. Above changes fixed this issue.
> 
> GEODE-2940 Now we don't validate configure host string on start. As
> there is possibility that host may not available.
> 
> 1. Earlier wan config were failing because of that. Now we just keep
> those configure host string. And try this later.
> 
> Added unit tests for it.
> 
> 
> Diffs
> -
> 
>   geode-assembly/build.gradle 39bb542 
>   geode-assembly/src/test/resources/expected_jars.txt 6260167 
>   geode-core/build.gradle 7f34b4a 
>   
> geode-core/src/main/java/org/apache/geode/admin/internal/DistributionLocatorImpl.java
>  c1bfc93 
>   
> geode-core/src/main/java/org/apache/geode/cache/client/internal/AutoConnectionSourceImpl.java
>  070451c 
>   
> geode-core/src/main/java/org/apache/geode/cache/client/internal/PoolImpl.java 
> 3ded54a 
>   
> geode-core/src/main/java/org/apache/geode/distributed/internal/AbstractDistributionConfig.java
>  01c6157 
>   
> geode-core/src/main/java/org/apache/geode/distributed/internal/InternalDistributedSystem.java
>  1572355 
>   
> geode-core/src/main/java/org/apache/geode/distributed/internal/membership/gms/GMSUtil.java
>  c6bef57 
>   
> geode-core/src/main/java/org/apache/geode/distributed/internal/membership/gms/locator/GMSLocator.java
>  93fa9da 
>   
> geode-core/src/main/java/org/apache/geode/distributed/internal/membership/gms/membership/GMSJoinLeave.java
>  84d42cf 
>   
> geode-core/src/main/java/org/apache/geode/distributed/internal/membership/gms/membership/HostAddress.java
>  PRE-CREATION 
>   
> geode-core/src/main/java/org/apache/geode/distributed/internal/tcpserver/TcpClient.java
>  e9476b5 
>   
> geode-core/src/main/java/org/apache/geode/internal/admin/remote/DistributionLocatorId.java
>  5ab1bed 
>   
> geode-core/src/main/java/org/apache/geode/internal/admin/remote/RemoteTransportConfig.java
>  1dc2fd1 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/ClusterConfigurationLoader.java
>

Review Request 60451: GEODE-2996: adding Put handler

[Brian Rowe]

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

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


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


Repository: geode


Description
---

This is a continuation of the review Alex submitted this morning with the 
following changes:

Addresses review feedback for GEODE-2996, mainly refactoring getOpertionHandler 
to handle failures like the putOperationHandler
Adding put operations to the RoundTripCacheConnectionJUnitTest, which is the 
integration test for the protobuf module
Removing service loading for protobuf operations and instead have the 
ProtobufStreamProcessor populate its OperationHandlerRegistry
Remove exception throwing from OperationHandler.process calls and remove 
TypeEncodingException
Fixing ProtobufOpsProcessor to handle responses for types other than get


Diffs
-

  
geode-protobuf/src/main/java/org/apache/geode/protocol/operations/OperationHandler.java
 7683e3bf3 
  
geode-protobuf/src/main/java/org/apache/geode/protocol/operations/registry/OperationsHandlerRegistry.java
 8e3a33149 
  
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufOpsProcessor.java
 d426149e4 
  
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufStreamProcessor.java
 d7b5d4bd2 
  
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufUtilities.java
 d76366298 
  
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetRequestOperationHandler.java
 d9c14752f 
  
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/PutRequestOperationHandler.java
 PRE-CREATION 
  
geode-protobuf/src/main/java/org/apache/geode/serialization/exception/TypeEncodingException.java
 f3145a774 
  
geode-protobuf/src/main/java/org/apache/geode/serialization/exception/UnsupportedEncodingTypeException.java
 c577e768a 
  
geode-protobuf/src/main/java/org/apache/geode/serialization/registry/exception/CodecNotRegisteredForTypeException.java
 5c923a520 
  geode-protobuf/src/main/proto/region_API.proto 52291c451 
  geode-protobuf/src/test/java/org/apache/geode/protocol/MessageUtil.java 
f0b0b417b 
  
geode-protobuf/src/test/java/org/apache/geode/protocol/RoundTripCacheConnectionJUnitTest.java
 b9faca3c9 
  
geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/ProtobufOpsProcessorJUnitTest.java
 fc980aec9 
  
geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/GetRequestOperationHandlerJUnitTest.java
 daa5870ed 
  
geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/PutRequestOperationHandlerJUnitTest.java
 PRE-CREATION 


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


Testing
---

Unit tests, whole module test, precheckin in progress.


Thanks,

Brian Rowe

Review Request 60442: GEODE-3130: Refactoring AcceptorImpl communication mode switch

[Brian Rowe]

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

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


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


Repository: geode


Description
---

This review addresses Udo's last feedback for GEODE-2995.  This change also 
cleaned up the imports on the AcceptorImplJUnitTest.


Diffs
-

  
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/AcceptorImpl.java
 50f7006fa 
  
geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/AcceptorImplJUnitTest.java
 7aa11b7ca 


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


Testing
---

Precheckin in progress.


Thanks,

Brian Rowe

Re: Review Request 60217: GEODE-2995: adding in protocol handler for protobuf messages

[Brian Rowe]

/org.apache.geode.protocol.operations.OperationHandler
 PRE-CREATION 
  
geode-protobuf/src/main/resources/META-INF/services/org.apache.geode.protocol.serializer.ProtocolSerializer
 PRE-CREATION 
  
geode-protobuf/src/main/resources/META-INF/services/org.apache.geode.serialization.TypeCodec
 PRE-CREATION 
  
geode-protobuf/src/test/java/org/apache/geode/protocol/IntegrationJUnitTest.java
 PRE-CREATION 
  geode-protobuf/src/test/java/org/apache/geode/protocol/MessageUtil.java 
PRE-CREATION 
  geode-protobuf/src/test/java/org/apache/geode/protocol/OpsHandler.java 
PRE-CREATION 
  
geode-protobuf/src/test/java/org/apache/geode/protocol/operations/registry/OperationsHandlerRegistryJUnitTest.java
 PRE-CREATION 
  
geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/EncodingTypeToSerializationTypeTranslatorJUnitTest.java
 PRE-CREATION 
  
geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/ProtobufOpsProcessorJUnitTest.java
 PRE-CREATION 
  
geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/GetRequestOperationHandlerJUnitTest.java
 PRE-CREATION 
  
geode-protobuf/src/test/java/org/apache/geode/protocol/serializer/ProtobufProtocolSerializerJUnitTest.java
 PRE-CREATION 
  
geode-protobuf/src/test/java/org/apache/geode/serialization/ProtobufSerializationServiceJUnitTest.java
 PRE-CREATION 
  
geode-protobuf/src/test/java/org/apache/geode/serialization/codec/BinaryFormatJUnitTest.java
 PRE-CREATION 
  
geode-protobuf/src/test/java/org/apache/geode/serialization/registry/CodecRegistryJUnitTest.java
 PRE-CREATION 
  gradle/dependency-versions.properties 183dafcc6 
  gradle/rat.gradle 1bea5843b 
  settings.gradle c0fdb6e4f 


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

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


Testing
---

Precheckin in progress.  Unit tests added for all new classes, integration test 
added for entire module.


Thanks,

Brian Rowe

Re: Review Request 60217: GEODE-2995: adding in protocol handler for protobuf messages

[Brian Rowe]

 
  geode-protobuf/src/main/proto/server_API.proto PRE-CREATION 
  
geode-protobuf/src/main/resources/META-INF/services/org.apache.geode.protocol.operations.OperationHandler
 PRE-CREATION 
  
geode-protobuf/src/main/resources/META-INF/services/org.apache.geode.protocol.serializer.ProtocolSerializer
 PRE-CREATION 
  
geode-protobuf/src/main/resources/META-INF/services/org.apache.geode.serialization.TypeCodec
 PRE-CREATION 
  
geode-protobuf/src/test/java/org/apache/geode/client/protocol/IntegrationTest.java
 PRE-CREATION 
  
geode-protobuf/src/test/java/org/apache/geode/client/protocol/MessageUtil.java 
PRE-CREATION 
  geode-protobuf/src/test/java/org/apache/geode/client/protocol/OpsHandler.java 
PRE-CREATION 
  
geode-protobuf/src/test/java/org/apache/geode/protocol/operations/protobuf/GetRequestOperationHandlerTest.java
 PRE-CREATION 
  
geode-protobuf/src/test/java/org/apache/geode/protocol/operations/registry/OperationsHandlerRegistryJUnitTest.java
 PRE-CREATION 
  
geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/ProtobufOpsProcessorTest.java
 PRE-CREATION 
  
geode-protobuf/src/test/java/org/apache/geode/protocol/serializer/ProtobufProtocolSerializerJUnitTest.java
 PRE-CREATION 
  
geode-protobuf/src/test/java/org/apache/geode/serialization/ProtobufSerializationServiceImplTest.java
 PRE-CREATION 
  
geode-protobuf/src/test/java/org/apache/geode/serialization/codec/StringCodecJUnitTest.java
 PRE-CREATION 
  
geode-protobuf/src/test/java/org/apache/geode/serialization/protobuf/translation/EncodingTypeToSerializationTypeTranslatorJUnitTest.java
 PRE-CREATION 
  
geode-protobuf/src/test/java/org/apache/geode/serialization/registry/CodecRegistryJUnitTest.java
 PRE-CREATION 
  gradle/rat.gradle 1bea5843b 
  settings.gradle c0fdb6e4f 


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

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


Testing
---

Precheckin in progress.  Unit tests added for all new classes, integration test 
added for entire module.


Thanks,

Brian Rowe

Re: Review Request 60217: GEODE-2995: adding in protocol handler for protobuf messages

[Brian Rowe]

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




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

Good catch, we shouldn't be catching anything here.



geode-protobuf/src/main/java/org/apache/geode/protocol/handler/ProtobufStreamProcessor.java
Lines 35 (patched)
<https://reviews.apache.org/r/60217/#comment252357>

It looks like the only state being maintained in here is the mapping of op 
code to op handler and for serialization type to codec, both of which are set 
up at construction time.  There shouldn't be any problem calling into this from 
multiple threads.



geode-protobuf/src/main/java/org/apache/geode/protocol/handler/ProtobufStreamProcessor.java
Lines 49 (patched)
<https://reviews.apache.org/r/60217/#comment252370>

Moved this to ...protocol.protobuf.  Within that package I created 
protobuf.handler and protobuf.operations and moved in the protobuf classes from 
org.apache.geode.protocol.{handler,operations}.protobuf to keep all the 
protobuf code in one area.



geode-protobuf/src/main/java/org/apache/geode/protocol/handler/ProtocolHandler.java
Lines 23 (patched)
<https://reviews.apache.org/r/60217/#comment252371>

Good call, ProtocolSerializer is much more descriptive.  Changed.



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

This is only used during the constructor of the OperationHandlerRegistry so 
that it can register itself.  We'll probably remove the service loading code in 
the registry construction, which will lead to the removal of this as well.



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

Good call, changed this to InvalidProtocolMessageException.



geode-protobuf/src/main/java/org/apache/geode/protocol/operations/registry/OperationsHandlerRegistry.java
Lines 24 (patched)
<https://reviews.apache.org/r/60217/#comment252374>

It's actually only specific to the protobuf because the service loader will 
automatically register all of the protobuf operation handlers.  

Let's remove service loading here and just create a protobuf specific 
instance in the ProtobufStreamProcessor which will individually register the 
protobuf specific handlers.  Given that this is somewhat broader scope than the 
rest of the review changes, I'm going to leave this for a follow up push after 
the merge.



geode-protobuf/src/test/java/org/apache/geode/client/protocol/IntegrationTest.java
Lines 66 (patched)
<https://reviews.apache.org/r/60217/#comment252375>

How about testGetRequestProcessed?



geode-protobuf/src/test/java/org/apache/geode/client/protocol/IntegrationTest.java
Lines 67 (patched)
<https://reviews.apache.org/r/60217/#comment252376>

That's a lot simpler, thanks!



geode-protobuf/src/test/java/org/apache/geode/client/protocol/IntegrationTest.java
Lines 82 (patched)
<https://reviews.apache.org/r/60217/#comment252381>

Turn out this is pretty easy to check through Mockito, added.



geode-protobuf/src/test/java/org/apache/geode/protocol/handler/ProtobufProtocolHandlerJUnitTest.java
Lines 62 (patched)
<https://reviews.apache.org/r/60217/#comment252382>

Nice, this really cleans up the test a lot



geode-protobuf/src/test/java/org/apache/geode/serialization/ProtobufSerializationServiceImplTest.java
Lines 54 (patched)
<https://reviews.apache.org/r/60217/#comment252383>

Fixed



geode-protobuf/src/test/java/org/apache/geode/serialization/registry/CodecRegistryJUnitTest.java
Lines 35 (patched)
<https://reviews.apache.org/r/60217/#comment252384>

Without this, JUnit can't determine the category of this test - 
http://static.javadoc.io/org.powermock/powermock-core/1.6.5/org/powermock/core/classloader/annotations/PowerMockIgnore.html



geode-protobuf/src/test/java/org/apache/geode/serialization/registry/CodecRegistryJUnitTest.java
Lines 58 (patched)
<https://reviews.apache.org/r/60217/#comment252385>

fixed



settings.gradle
Line 39 (original), 39 (patched)
<https://reviews.apache.org/r/60217/#comment252354>

Alright, I'll leave that for that batch of work then


- Brian Rowe


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 60217: GEODE-2995: adding in protocol handler for protobuf messages

[Brian Rowe]

-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

Re: Review Request 59896: GEODE-3043 surprise member added when the member is already in the cluster

[Brian Rowe]

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


Ship it!




Ship It!

- Brian Rowe


On June 7, 2017, 10:02 p.m., Bruce Schuchardt wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59896/
> ---
> 
> (Updated June 7, 2017, 10:02 p.m.)
> 
> 
> Review request for geode, Alexander Murmann, Galen O'Sullivan, Hitesh 
> Khamesra, Udo Kohlmeyer, and Brian Rowe.
> 
> 
> Bugs: GEODE-3043
> https://issues.apache.org/jira/browse/GEODE-3043
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> Modified InternalDistributedMember to disregard the "name" field in compareTo 
> if partial IDs are being used in the comparison.
> 
> Modified GMSMembershipManager to attempt to replace partial IDs with the full 
> IDs from the membership view.  Typically these IDs will cause no problems and 
> methods in GMSMembershipManager that send messages to a recipient will also 
> attempt to replace the ID with a full ID from the membership view, but it's 
> good to try to keep IDs canonical.
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/main/java/org/apache/geode/distributed/internal/membership/InternalDistributedMember.java
>  6982d31f13ad5f5f004f0e7c9a0f4e1aa26a151a 
>   
> geode-core/src/main/java/org/apache/geode/distributed/internal/membership/gms/GMSMember.java
>  c82d97e9facca19bfb08b20407abf21dd274684e 
>   
> geode-core/src/main/java/org/apache/geode/distributed/internal/membership/gms/mgr/GMSMembershipManager.java
>  fe560d9eb71131cf1ca5e5ca124e877769e9d518 
>   
> geode-core/src/test/java/org/apache/geode/distributed/DistributedMemberDUnitTest.java
>  0153ca6506af574798ad01b98d9a395fad7be5d5 
> 
> 
> Diff: https://reviews.apache.org/r/59896/diff/1/
> 
> 
> Testing
> ---
> 
> New tests, precheckin
> 
> 
> Thanks,
> 
> Bruce Schuchardt
> 
>

Re: Review Request 59894: GEODE-3041 CI failure: DistributedMemberDUnitTest.testGroupsInAllVMs fails intermittently

[Brian Rowe]

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


Ship it!




Ship It!

- Brian Rowe


On June 7, 2017, 9:56 p.m., Bruce Schuchardt wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59894/
> ---
> 
> (Updated June 7, 2017, 9:56 p.m.)
> 
> 
> Review request for geode, Alexander Murmann, Galen O'Sullivan, Hitesh 
> Khamesra, Udo Kohlmeyer, and Brian Rowe.
> 
> 
> Bugs: GEODE-3041
> https://issues.apache.org/jira/browse/GEODE-3041
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> When shutting down the MembershipManager after it has joined we should not 
> use uncleanShutdown because the member will appear to have crashed.  Instead 
> we should do a normal shutdown.
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/main/java/org/apache/geode/distributed/internal/DistributionChannel.java
>  ef4056ca56c1b102e507f96bbfe41396d0139aa5 
>   
> geode-core/src/main/java/org/apache/geode/distributed/internal/DistributionManager.java
>  029e6377f56d80dd81e4cec430f106ac743e5178 
> 
> 
> Diff: https://reviews.apache.org/r/59894/diff/1/
> 
> 
> Testing
> ---
> 
> The test was failing every time I ran it.  It no longer fails.
> 
> 
> Thanks,
> 
> Bruce Schuchardt
> 
>

Re: Review Request 59925: GEODE-3052 Restarting 2 locators within 1s of each other causes potential locator split brain

[Brian Rowe]

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


Ship it!




Ship It!

- Brian Rowe


On June 8, 2017, 6:36 p.m., Bruce Schuchardt wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59925/
> ---
> 
> (Updated June 8, 2017, 6:36 p.m.)
> 
> 
> Review request for geode, Alexander Murmann, Galen O'Sullivan, Hitesh 
> Khamesra, Udo Kohlmeyer, and Brian Rowe.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> When restarting from a locatorView.dat file we should ignore any locator 
> entries in the view.  Recovery tries to get this state from other locators 
> before resorting to using the persisted view so there we know all of the 
> locator entries in the view are invalid.  This allows the locators to quickly 
> move into the concurrent-startup algorithm and find each other.
> 
> I removed the Flaky categorization of the test that I modified to reproduce 
> the problem.  A subclass's use of the test was reported as a Flaky failure 
> but I found that the ticket was closed.
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/main/java/org/apache/geode/distributed/internal/membership/gms/locator/GMSLocator.java
>  e3635f2d93aae212cbff2f2058b6dc728a04776a 
>   geode-core/src/test/java/org/apache/geode/distributed/LocatorDUnitTest.java 
> 8ff9b67e13dd50499d861ff62ddae3fb8668dd28 
>   
> geode-core/src/test/java/org/apache/geode/distributed/LocatorUDPSecurityDUnitTest.java
>  9d49d30abfb8acccd8a5547ba0ee3c7bcf9e7970 
> 
> 
> Diff: https://reviews.apache.org/r/59925/diff/1/
> 
> 
> Testing
> ---
> 
> The problem was easily reproduced using LocatorDUnitTest.testStartTwoLocators 
> by repeating the cycling of the locators.  It failed every time I ran it.
> 
> 
> Thanks,
> 
> Bruce Schuchardt
> 
>

Re: Review Request 59863: Removing obsolete SSL handling in `AcceptorImpl.accept` catch block

[Brian Rowe]

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

(Updated June 8, 2017, 12:40 a.m.)


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


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


Repository: geode


Description
---

SSL handshake is now done in a separate thread and will never reach the handler 
code which is being removed.


Diffs (updated)
-

  
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/AcceptorImpl.java
 9658f98da 


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

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


Testing
---

Precheckin started


Thanks,

Brian Rowe

Review Request 59863: Removing obsolete SSL handling in `AcceptorImpl.accept` catch block

[Brian Rowe]

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

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


Repository: geode


Description
---

SSL handshake is now done in a separate thread and will never reach the handler 
code which is being removed.


Diffs
-

  
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/AcceptorImpl.java
 9658f98da 


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


Testing
---

Precheckin started


Thanks,

Brian Rowe