> 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? > > Kirk Lund wrote: > 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.
If you envisioned that security service will be rippsed out of ds in the future, and wants to gear towards that way, then I can see your point. But from the current state of things I would not call it bad o-o design though. If I see any method call that's in the shape of callXYZ(o, o.getX(), o.getY(), o.getZ()), then I would question why bother pssing in the last three parameters. To me, that's anti-pattern. - Jinmei ----------------------------------------------------------- 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 > >
