----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60157/#review178387 -----------------------------------------------------------
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/AcceptorImpl.java Line 252 (original), 280 (patched) <https://reviews.apache.org/r/60157/#comment252314> Why are these all multiline comments now despite being only one line? Does the style guide demand this? geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/GenericProtocolServerConnection.java Lines 34 (patched) <https://reviews.apache.org/r/60157/#comment252316> Would it be better to track TODOs in Jira where we will for sure look at it and can prioritize it? geode-core/src/main/java/org/apache/geode/internal/security/CustomSecurityService.java Lines 117 (patched) <https://reviews.apache.org/r/60157/#comment252317> I am suprised this is log level debug and not info. Security events to me are alway something I want to have on record. You really need them when you didn't expect you'd need them because it's time to find out how your system was compromised. Hackers rarely change the debug level to make that easier for me. ;) geode-core/src/main/java/org/apache/geode/internal/security/CustomSecurityService.java Lines 307 (patched) <https://reviews.apache.org/r/60157/#comment252320> This section does something distinct enough that we are giving it a comment. Would this be worthwhile extracting it? It also seems to act on a slightly different abstraction level than other things in this method (excluding that string manipulation higher up). geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/ServerConnectionFactoryTest.java Lines 58 (patched) <https://reviews.apache.org/r/60157/#comment252325> We never test that any of the other values we pass in get set. - Alexander Murmann 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 > >
