> On June 19, 2017, 4:24 p.m., Udo Kohlmeyer wrote: > > 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/diff/2/?file=1752472#file1752472line1424> > > > > this conditional is becoming exponentially worse. Could we maybe > > extract this to a method/function?
I've made a switch statement above, which is much better. > On June 19, 2017, 4:24 p.m., Udo Kohlmeyer wrote: > > 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/diff/2/?file=1752472#file1752472line1427> > > > > This does not seem to be used anywhere anymore. Could you please check > > and remove This is used in the constructor for ServerConnection and goes into the thread name and possibly elsewhere. > On June 19, 2017, 4:24 p.m., Udo Kohlmeyer wrote: > > 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/diff/2/?file=1752472#file1752472line1428> > > > > 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 I made a switch statement, have a look again and see if you like it. > On June 19, 2017, 4:24 p.m., Udo Kohlmeyer wrote: > > 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/diff/2/?file=1752472#file1752472line1429> > > > > should we have a similar setting for the CLIENT_TO_SERVER_NEW_PROTOCOL? yes, good catch, thanks. Fixed. > On June 19, 2017, 4:24 p.m., Udo Kohlmeyer wrote: > > geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/NewProtocolServerConnection.java > > Lines 32 (patched) > > <https://reviews.apache.org/r/60157/diff/2/?file=1752476#file1752476line32> > > > > Maybe we rename this to GenericProtocolServerConnection I don't like the name because it's generic, but it works well enough. > On June 19, 2017, 4:24 p.m., Udo Kohlmeyer wrote: > > geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/NewProtocolServerConnection.java > > Lines 35 (patched) > > <https://reviews.apache.org/r/60157/diff/2/?file=1752476#file1752476line35> > > > > clientProtocolMessageHandler Renaming to `messageHandler`. > On June 19, 2017, 4:24 p.m., Udo Kohlmeyer wrote: > > geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/NewProtocolServerConnection.java > > Lines 54 (patched) > > <https://reviews.apache.org/r/60157/diff/2/?file=1752476#file1752476line54> > > > > could we rename this to clientProtocolHandler or > > clientProtocolMessageHandler instead of newClientProtocol That name doesn't give the user any information.. I like `protobufProtocolHandler` better. > On June 19, 2017, 4:24 p.m., Udo Kohlmeyer wrote: > > geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/NewProtocolServerConnection.java > > Lines 58 (patched) > > <https://reviews.apache.org/r/60157/diff/2/?file=1752476#file1752476line58> > > > > surely we don't need this here... How do we handle the case if we have > > another client protocol handler... this would fail. We can test this at the factory level. I hadn't been thinking of this as being so generic, but it can be pretty easily. - Galen ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60157/#review178152 ----------------------------------------------------------- On June 20, 2017, 12:53 a.m., Galen O'Sullivan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60157/ > ----------------------------------------------------------- > > (Updated June 20, 2017, 12:53 a.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/distributed/internal/InternalDistributedSystem.java > 85f914637 > > geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java > 5eaa5a4bd > 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/GenericProtocolServerConnection.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/ServerConnection.java > 947b83652 > > geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ServerConnectionFactory.java > PRE-CREATION > > geode-core/src/main/java/org/apache/geode/internal/security/CallbackInstantiator.java > 2cb8d0817 > > geode-core/src/main/java/org/apache/geode/internal/security/CustomSecurityService.java > PRE-CREATION > > geode-core/src/main/java/org/apache/geode/internal/security/DisabledSecurityService.java > PRE-CREATION > > geode-core/src/main/java/org/apache/geode/internal/security/IntegratedSecurityService.java > 171cfb71a > > geode-core/src/main/java/org/apache/geode/internal/security/LegacySecurityService.java > c594bf932 > > geode-core/src/main/java/org/apache/geode/internal/security/SecurityService.java > feea8995d > > geode-core/src/main/java/org/apache/geode/internal/security/SecurityServiceFactory.java > 2e0ad956c > > geode-core/src/main/java/org/apache/geode/internal/security/SecurityServiceType.java > PRE-CREATION > > geode-core/src/main/java/org/apache/geode/internal/security/shiro/ConfigInitializer.java > PRE-CREATION > > geode-core/src/main/java/org/apache/geode/internal/security/shiro/CustomAuthRealm.java > 0e5029bd0 > > geode-core/src/main/java/org/apache/geode/internal/security/shiro/SecurityManagerProvider.java > ad8e66e0c > > geode-core/src/test/java/org/apache/geode/distributed/internal/membership/MembershipJUnitTest.java > b0e20d9b3 > > 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 > > geode-core/src/test/java/org/apache/geode/internal/security/DisabledSecurityServiceTest.java > PRE-CREATION > > geode-core/src/test/java/org/apache/geode/internal/security/FakePostProcessor.java > PRE-CREATION > > geode-core/src/test/java/org/apache/geode/internal/security/FakeSecurityManager.java > PRE-CREATION > > geode-core/src/test/java/org/apache/geode/internal/security/IntegratedSecurityServiceConstructorTest.java > afa007f31 > > geode-core/src/test/java/org/apache/geode/internal/security/IntegratedSecurityServiceTest.java > daaf18d3f > > geode-core/src/test/java/org/apache/geode/internal/security/LegacySecurityServiceTest.java > bac79ec0e > > geode-core/src/test/java/org/apache/geode/internal/security/SecurityServiceFactoryShiroIntegrationTest.java > e8548ed86 > > geode-core/src/test/java/org/apache/geode/internal/security/SecurityServiceFactoryTest.java > fc4447bb0 > > geode-core/src/test/java/org/apache/geode/internal/security/SecurityServiceTest.java > 4b7bbfc5a > > geode-core/src/test/java/org/apache/geode/internal/security/shiro/ConfigInitializerIntegrationTest.java > PRE-CREATION > > geode-core/src/test/java/org/apache/geode/management/internal/security/SecurityServiceWithCustomRealmIntegrationTest.java > c47432bbc > > geode-core/src/test/java/org/apache/geode/management/internal/security/SecurityServiceWithShiroIniIntegrationTest.java > 86a0ff0ed > > geode-core/src/test/java/org/apache/geode/security/CacheFactoryWithSecurityObjectTest.java > 94e0be5e0 > > geode-core/src/test/java/org/apache/geode/security/SecurityManagerLifecycleDistributedTest.java > 872740645 > geode-docs/developing/transactions/JTA_transactions.html.md.erb ffb6082cc > geode-pulse/src/test/java/org/apache/geode/tools/pulse/tests/Server.java > ad5c0806b > > geode-pulse/src/test/java/org/apache/geode/tools/pulse/tests/rules/ServerRule.java > f62bb7489 > > > Diff: https://reviews.apache.org/r/60157/diff/3/ > > > 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 > >
