geode-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alexander Murmann <amurm...@pivotal.io>
Subject Re: Review Request 60157: GEODE-3075: initial work for feature flag and creation of a new subclass of `ServerConnection`.
Date Tue, 20 Jun 2017 15:54:12 GMT

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


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