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