> On June 26, 2017, 11:58 p.m., Jinmei Liao wrote: > > geode-core/src/main/java/org/apache/geode/cache/client/internal/ConnectionFactoryImpl.java > > Line 82 (original), 82 (patched) > > <https://reviews.apache.org/r/60375/diff/1/?file=1763105#file1763105line82> > > > > since we already pass in the entire sys, do we need to pass in > > sys.getSecurityService() as well? Can we change the signaure of the > > constructor to take an InternalDistributedSystem, and directly uses the > > security service inside the passed sys?
I'm trying to separate our major services instead of folding them in together. For example, I don't want to go to Cache to get DistributedSystem (but you can) and I don't want our code to go to DistributedSystem to get SecurityService. The link between DistributedSystem and SecurityService is tenuous at best -- DistributedSystem USES SecurityService but it doesn't logically own it except for the fact that DS is the 1st object in Geode that needs to use it. So from the point-of-view of an object like HandShake, I'm trying to give it individual services. This also has the side effect of making classes such as HandShake easier to unit test when mocking the collaborators. Another reason I'm doing this is because it makes ALL of the collorators visible at the constructor level which is a good practice to follow. If we want to start using Dependency Injection (which I want to), then we need to do even more of this -- passing in every collaborator individually rather than passing in a god object to invoke getters against on it to get the other dependencies. Or to summarize even more, I'm trying to steer away from bad code to good code (good object-oriented design) and god objects are one of the worst anti-patterns. - Kirk ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60375/#review178940 ----------------------------------------------------------- On June 26, 2017, 6:02 p.m., Kirk Lund wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60375/ > ----------------------------------------------------------- > > (Updated June 26, 2017, 6:02 p.m.) > > > Review request for geode, Emily Yeh, Jinmei Liao, Jared Stewart, Ken Howe, > and Patrick Rhomberg. > > > Bugs: GEODE-3117 > https://issues.apache.org/jira/browse/GEODE-3117 > > > Repository: geode > > > Description > ------- > > GEODE-3117: fix Gateway authentication with legacy security > > * add GatewayLegacyAuthenticationRegressionTest to reproduce bug > * fix authentication of Gateway sender/receiver with > SECURITY_CLIENT_AUTHENTICATOR > > > Diffs > ----- > > > geode-core/src/main/java/org/apache/geode/cache/client/internal/ConnectionFactoryImpl.java > a419d575010d39d4dab6c5c8f9748928c1764344 > > geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/HandShake.java > 32735b9ab17fe9467cea46096bd177902145e4bd > > geode-wan/src/test/java/org/apache/geode/internal/cache/wan/misc/GatewayLegacyAuthenticationRegressionTest.java > PRE-CREATION > > > Diff: https://reviews.apache.org/r/60375/diff/1/ > > > Testing > ------- > > * new test reproduces GEODE-3117: GatewayLegacyAuthenticationRegressionTest > * precheckin passes > > > Thanks, > > Kirk Lund > >
