geode-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Bruce Schuchardt <bschucha...@pivotal.io>
Subject Re: Review Request 60157: GEODE-3075: initial work for feature flag and creation of a new subclass of `ServerConnection`.
Date Mon, 19 Jun 2017 18:40:52 GMT

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



I just have a few small things.


geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/AcceptorImpl.java
Line 1426 (original), 1426 (patched)
<https://reviews.apache.org/r/60157/#comment252160>

    It seems to me that a protobuf client connection should be reported as such and not as
an empty string.



geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/ServerConnectionFactoryIntegrationTest.java
Lines 45 (patched)
<https://reviews.apache.org/r/60157/#comment252167>

    Use the RestoreSystemProperties junit rule here & remove the try/finally



geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/ServerConnectionFactoryTest.java
Lines 47 (patched)
<https://reviews.apache.org/r/60157/#comment252169>

    Since you're testing both with & without the system property being set you should
add a try/finally to this test to clear the property.


- Bruce Schuchardt


On June 16, 2017, 5:14 p.m., Galen O'Sullivan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60157/
> -----------------------------------------------------------
> 
> (Updated June 16, 2017, 5:14 p.m.)
> 
> 
> Review request for geode, Alexander Murmann, Bruce Schuchardt, Hitesh Khamesra, Udo Kohlmeyer,
and Brian Rowe.
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> Create `ServerConnectionFactory`, which creates either instances of `LegacyServerConnection`
(identical in functionality to the old `ServerConnection`) or `NewProtocolServerConnection`
(which is currently basically a stub, but will never get called unless a feature flag is set).
> 
> This is the initial work for GEODE-3074, and will allow us to continue to work on further
tasks for that project.
> 
> An explicit goal with this PR is to not disturb any existing functionality. Unless a
feature flag is set and a connection with a certain magic byte is received, server connections
will work as before. If you see something that you think may break, please let me know.
> 
> Have a nice day!
> 
> 
> Diffs
> -----
> 
>   geode-core/src/main/java/org/apache/geode/internal/cache/tier/Acceptor.java 9a3241b05

>   geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/AcceptorImpl.java
2a8818cef 
>   geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ClientHealthMonitor.java
e0b5ab8b6 
>   geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ClientProtocolMessageHandler.java
PRE-CREATION 
>   geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/LegacyServerConnection.java
PRE-CREATION 
>   geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/NewProtocolServerConnection.java
PRE-CREATION 
>   geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ServerConnection.java
947b83652 
>   geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ServerConnectionFactory.java
PRE-CREATION 
>   geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/ServerConnectionFactoryIntegrationTest.java
PRE-CREATION 
>   geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/ServerConnectionFactoryTest.java
PRE-CREATION 
>   geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/ServerConnectionTest.java
794c61097 
> 
> 
> Diff: https://reviews.apache.org/r/60157/diff/2/
> 
> 
> Testing
> -------
> 
> precheckin passed (on a version without the integration test, but I'd consider it pretty
much equivalent. If you want me to run again, I will.)
> 
> 
> Thanks,
> 
> Galen O'Sullivan
> 
>


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