----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60157/#review178152 -----------------------------------------------------------
geode-core/src/main/java/org/apache/geode/internal/cache/tier/Acceptor.java Lines 77 (patched) <https://reviews.apache.org/r/60157/#comment252002> Could we rename this to PROTOBUF_CLIENT_SERVER_PROTOCOL? geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/AcceptorImpl.java Line 1372 (original), 1372 (patched) <https://reviews.apache.org/r/60157/#comment252141> pls rename to s = socket geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/AcceptorImpl.java Line 1380 (original), 1380 (patched) <https://reviews.apache.org/r/60157/#comment252140> please rename to useful field. sc = socketChannel geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/AcceptorImpl.java Line 1390 (original), 1390 (patched) <https://reviews.apache.org/r/60157/#comment252142> st = timerTask.. Pls rename geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/AcceptorImpl.java Lines 1424-1426 (original), 1424-1426 (patched) <https://reviews.apache.org/r/60157/#comment252135> this conditional is becoming exponentially worse. Could we maybe extract this to a method/function? geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/AcceptorImpl.java Line 1427 (original), 1427 (patched) <https://reviews.apache.org/r/60157/#comment252139> This does not seem to be used anywhere anymore. Could you please check and remove geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/AcceptorImpl.java Lines 1428-1441 (original), 1428-1441 (patched) <https://reviews.apache.org/r/60157/#comment252136> Doubt that this only needs to happen inside of this conditional. Maybe we extract this out into its own method and then call it outside of the method geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/AcceptorImpl.java Lines 1429-1431 (original), 1429-1431 (patched) <https://reviews.apache.org/r/60157/#comment252003> should we have a similar setting for the CLIENT_TO_SERVER_NEW_PROTOCOL? geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/NewProtocolServerConnection.java Lines 32 (patched) <https://reviews.apache.org/r/60157/#comment252146> Maybe we rename this to GenericProtocolServerConnection geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/NewProtocolServerConnection.java Lines 35 (patched) <https://reviews.apache.org/r/60157/#comment252147> clientProtocolMessageHandler geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/NewProtocolServerConnection.java Lines 54 (patched) <https://reviews.apache.org/r/60157/#comment252132> could we rename this to clientProtocolHandler or clientProtocolMessageHandler instead of newClientProtocol geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/NewProtocolServerConnection.java Lines 58 (patched) <https://reviews.apache.org/r/60157/#comment252133> surely we don't need this here... How do we handle the case if we have another client protocol handler... this would fail. - Udo Kohlmeyer 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 > >
