geode-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Galen O'Sullivan <gosulli...@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 00:54:21 GMT


> On June 19, 2017, 6:40 p.m., Bruce Schuchardt wrote:
> > 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/diff/2/?file=1752472#file1752472line1426>
> >
> >     It seems to me that a protobuf client connection should be reported as such
and not as an empty string.

good catch, fixed.


> On June 19, 2017, 6:40 p.m., Bruce Schuchardt wrote:
> > geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/ServerConnectionFactoryIntegrationTest.java
> > Lines 45 (patched)
> > <https://reviews.apache.org/r/60157/diff/2/?file=1752479#file1752479line45>
> >
> >     Use the RestoreSystemProperties junit rule here & remove the try/finally

good call, fixed.


> On June 19, 2017, 6:40 p.m., Bruce Schuchardt wrote:
> > geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/ServerConnectionFactoryTest.java
> > Lines 47 (patched)
> > <https://reviews.apache.org/r/60157/diff/2/?file=1752480#file1752480line47>
> >
> >     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.

Missed that. Good catch.


- Galen


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60157/#review178279
-----------------------------------------------------------


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