> 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. > > Jinmei Liao wrote: > 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.
I'd much prefer to discuss object-oriented design principles outside of a simple bug fix. You seem to not want me to make subtle improvements to class design. YOu need to understand that I can't fix every class in Geode all at once -- I can only do it very gradually. DistributedSystem is a god object [1] which is a pattern that we want to move away from. Requiring HandShake to know that DistributedSystem (2nd arg to constructor) has a SecurityService breaks the Law of Demeter [2]. As we change classes to use Dependency Injection [3], one visible difference. is that you're going to see every logically separate dependency as a separate argument being passed into the constructor. Now ask yourself, is SecurityService logically a part of DistributedSystem? Such that it doesn't make sense to use Security outside of the membership and distribution code? Last but not least, we have organized our teams to utilize what upper management calls Inverse Conway's Manuever [4]. Unfortunately this point has more to do with Pivotal and GemFire than Geode. The point of Inverse Conway's Manuever (according to Elisabeth and Michael) is that our code will gradually separate from the code owned by other teams and eventually become so modular that we can even have separate git repos. The Communications team owns membership and distribution (org.apache.geode.distributed). The Storage team owns caching (org.apache.geode.cache). The M&M team owns Security (org.apache.geode.security) and Configuration. Unfortunately Configuration is currently spaghettied throughout DistributedSystem and Cache (distributed and cache packages). The only reason why InternalDistributedSystem currently touches SecurityService is because it needs to inject SecurityService into the membership Services. We would potentially create a new Configuration entity which handles the li fecycle of configuration in Geode instead of it being spread out within DistributedSystem and Cache. So aside from breaking Law of Demeter, using system.getSecurityService() instead of simply passing in SecurityService adds one more point of code that we have to change when we tease apart SecurityService from both DistributedSystem and Cache. I honestly don't know what else to tell you regarding this. I believe we need to stop having these reviews break down into philosophical discussions about proper object oriented coding and how each person involved on Geode wants to guide the code as it evolves towards better modularity. Any questions about why I'm trying to do this? [1] https://en.wikipedia.org/wiki/God_object [2] https://en.wikipedia.org/wiki/Law_of_Demeter [3] https://en.wikipedia.org/wiki/Dependency_injection [4] https://www.thoughtworks.com/radar/techniques/inverse-conway-maneuver - Kirk ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60375/#review178940 ----------------------------------------------------------- On June 29, 2017, 5:29 p.m., Kirk Lund wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60375/ > ----------------------------------------------------------- > > (Updated June 29, 2017, 5:29 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/2/ > > > Testing > ------- > > * new test reproduces GEODE-3117: GatewayLegacyAuthenticationRegressionTest > * precheckin passes > > > Thanks, > > Kirk Lund > >
