Re: [DISCUSS] Add a test dependency to geode-core - ArchUnit
I like the idea especially if its only for compile/test time and doesn't make it into the binary bits. On Fri, Jun 21, 2019 at 1:04 PM Udo Kohlmeyer wrote: > Thank you for the response... > > #1 - But isn't cyclical package dependent code not a smell and practice, > whilst at the same time and uni-directional dependency is preferred. > > Soo... I think I see the benefit to be more, that ArchUnit allows the > untangling of code into a modular way WITHOUT a big bang approach of > moving the code into modules and then having to be concerned about the > fallout. But also it allows for the managing of package dependencies > WITHOUT breaking the code out into different separate modules. > > I really like ArchUnit :).. We should prioritize adoption :) > > --Udo > > On 6/21/19 12:48, Murtuza Boxwala wrote: > > Two things come to mind: > > > > 1) uni-directional dependency > > Packages can be dependent on each other, because the classes inside of > them can use each other. e.g. let’s say package A has class A1 and class > A2 and package B has class B1 and B2. A1 can depend on B1, and B2 can > depend on A2. Hence, the packages are dependent on each other. > > > > Modules can only have uni-directional dependency. If Module A depends on > Module B, then no class in Module B can reference a class in Module A. > This prevents tangling, i.e. spaghetti > > > > 2) Incremental compilation > > This lack of tangling helps not only developers, but the compiler too. > In the packages example above, if I change any of the classes, all the code > has to get recompiled because the dependency lines can go in any direction, > and the compiler won’t attempt to optimize. In the modules case, if Module > A changes, Module B will not recompile, because the dependency guarantees > that nothing about Module B could have been affected. > > > >> On Jun 21, 2019, at 2:14 PM, Udo Kohlmeyer wrote: > >> > >> I know that I'm missing the benefit of physically moving the code from > the package into its own module. > >> > >> Could you possibly explain to me what it is? > >> > >> On 6/21/19 07:37, Murtuza Boxwala wrote: > >>> I think that’s a really clever way to increment toward splitting > geode-core into more modules. I am excited to see what it looks like 👍 > >>> > On Jun 20, 2019, at 7:45 PM, Jacob Barrett > wrote: > > Gotcha! Sounds good. > > > On Jun 20, 2019, at 4:35 PM, Dan Smith wrote: > > > > We don't have a membership gradle module, just a package. We're > adding this > > to geode-core. > > > > For a little more context - we are thinking about refactoring > membership > > (and/or maybe some other pieces) into separate gradle modules - > proposal > > forthcoming! However, as a first step we need to untangle those > pieces of > > code from the rest of geode-core. Rather than creating some long > lived > > branch we can incrementally untangle the code a piece at a time, on > > develop. Having a way to track progress and enforce the direction of > > dependencies on the way to a separate gradle module will help with > that. > > > > -Dan > > > >> On Thu, Jun 20, 2019 at 4:23 PM Jacob Barrett > wrote: > >> > >> Are you adding this dependency to just the membership module? I am > cool > >> with that. > >> > >>> On Jun 20, 2019, at 2:39 PM, Dan Smith wrote: > >>> > >>> Hi all, > >>> > >>> Bill, Ernie, and I would like to add a new (apache licensed) test > >>> dependency to geode-core - https://github.com/TNG/ArchUnit. This > is a > >> tool > >>> that lets you write tests that make assertions about the > >> interdependencies > >>> of your code - for example enforcing that package A does not > depend on > >>> package B. > >>> > >>> Initially we intend to add some tests about what parts of the > system the > >>> org.apache.geode.distributed.internal.membership package depends > on, with > >>> an eye towards making that code more independently testable > (proposal on > >>> that coming soon!). > >>> > >>> Does anyone have an issue with adding this test dependency? > >>> > >>> -Dan > > > -- Charlie Black | cbl...@pivotal.io
Re: [DISCUSS] Add a test dependency to geode-core - ArchUnit
Of equal importance to uni-directional dependencies in a "modular" design is, classes in package A should not refer directly to classes in package B when A depends on B, or alternately, when module A depends on module B. All interactions are only ever through interfaces and all implementations are provided via an SPI, Factory, DI, etc, only. This makes B swappable with another implementation similar to B that honors the contract of B. Nearly all good Java framework are this way, e.g. Logging framework facades. 1 interface for logging concerns, multiple implementations. A superset of tangling is coupling and coupling is far worse than tangling. Tangling, while discourages, can be manageable where as coupling is much harder to uncouple. Still, a huge +1 to uni-directional dependencies, which is often achieved with organization/structuring and "strong" *Separation of Concerns*. Cheers! On Fri, Jun 21, 2019 at 1:04 PM Udo Kohlmeyer wrote: > Thank you for the response... > > #1 - But isn't cyclical package dependent code not a smell and practice, > whilst at the same time and uni-directional dependency is preferred. > > Soo... I think I see the benefit to be more, that ArchUnit allows the > untangling of code into a modular way WITHOUT a big bang approach of > moving the code into modules and then having to be concerned about the > fallout. But also it allows for the managing of package dependencies > WITHOUT breaking the code out into different separate modules. > > I really like ArchUnit :).. We should prioritize adoption :) > > --Udo > > On 6/21/19 12:48, Murtuza Boxwala wrote: > > Two things come to mind: > > > > 1) uni-directional dependency > > Packages can be dependent on each other, because the classes inside of > them can use each other. e.g. let’s say package A has class A1 and class > A2 and package B has class B1 and B2. A1 can depend on B1, and B2 can > depend on A2. Hence, the packages are dependent on each other. > > > > Modules can only have uni-directional dependency. If Module A depends on > Module B, then no class in Module B can reference a class in Module A. > This prevents tangling, i.e. spaghetti > > > > 2) Incremental compilation > > This lack of tangling helps not only developers, but the compiler too. > In the packages example above, if I change any of the classes, all the code > has to get recompiled because the dependency lines can go in any direction, > and the compiler won’t attempt to optimize. In the modules case, if Module > A changes, Module B will not recompile, because the dependency guarantees > that nothing about Module B could have been affected. > > > >> On Jun 21, 2019, at 2:14 PM, Udo Kohlmeyer wrote: > >> > >> I know that I'm missing the benefit of physically moving the code from > the package into its own module. > >> > >> Could you possibly explain to me what it is? > >> > >> On 6/21/19 07:37, Murtuza Boxwala wrote: > >>> I think that’s a really clever way to increment toward splitting > geode-core into more modules. I am excited to see what it looks like 👍 > >>> > On Jun 20, 2019, at 7:45 PM, Jacob Barrett > wrote: > > Gotcha! Sounds good. > > > On Jun 20, 2019, at 4:35 PM, Dan Smith wrote: > > > > We don't have a membership gradle module, just a package. We're > adding this > > to geode-core. > > > > For a little more context - we are thinking about refactoring > membership > > (and/or maybe some other pieces) into separate gradle modules - > proposal > > forthcoming! However, as a first step we need to untangle those > pieces of > > code from the rest of geode-core. Rather than creating some long > lived > > branch we can incrementally untangle the code a piece at a time, on > > develop. Having a way to track progress and enforce the direction of > > dependencies on the way to a separate gradle module will help with > that. > > > > -Dan > > > >> On Thu, Jun 20, 2019 at 4:23 PM Jacob Barrett > wrote: > >> > >> Are you adding this dependency to just the membership module? I am > cool > >> with that. > >> > >>> On Jun 20, 2019, at 2:39 PM, Dan Smith wrote: > >>> > >>> Hi all, > >>> > >>> Bill, Ernie, and I would like to add a new (apache licensed) test > >>> dependency to geode-core - https://github.com/TNG/ArchUnit. This > is a > >> tool > >>> that lets you write tests that make assertions about the > >> interdependencies > >>> of your code - for example enforcing that package A does not > depend on > >>> package B. > >>> > >>> Initially we intend to add some tests about what parts of the > system the > >>> org.apache.geode.distributed.internal.membership package depends > on, with > >>> an eye towards making that code more independently testable > (proposal on > >>> that coming soon!). > >>> > >>> Does anyone have an issue with adding this test dependency? > >>> > >>>
Re: [DISCUSS] Add a test dependency to geode-core - ArchUnit
Thank you for the response... #1 - But isn't cyclical package dependent code not a smell and practice, whilst at the same time and uni-directional dependency is preferred. Soo... I think I see the benefit to be more, that ArchUnit allows the untangling of code into a modular way WITHOUT a big bang approach of moving the code into modules and then having to be concerned about the fallout. But also it allows for the managing of package dependencies WITHOUT breaking the code out into different separate modules. I really like ArchUnit :).. We should prioritize adoption :) --Udo On 6/21/19 12:48, Murtuza Boxwala wrote: Two things come to mind: 1) uni-directional dependency Packages can be dependent on each other, because the classes inside of them can use each other. e.g.let’s say package A has class A1 and class A2 and package B has class B1 and B2. A1 can depend on B1, and B2 can depend on A2. Hence, the packages are dependent on each other. Modules can only have uni-directional dependency. If Module A depends on Module B, then no class in Module B can reference a class in Module A. This prevents tangling, i.e. spaghetti 2) Incremental compilation This lack of tangling helps not only developers, but the compiler too. In the packages example above, if I change any of the classes, all the code has to get recompiled because the dependency lines can go in any direction, and the compiler won’t attempt to optimize. In the modules case, if Module A changes, Module B will not recompile, because the dependency guarantees that nothing about Module B could have been affected. On Jun 21, 2019, at 2:14 PM, Udo Kohlmeyer wrote: I know that I'm missing the benefit of physically moving the code from the package into its own module. Could you possibly explain to me what it is? On 6/21/19 07:37, Murtuza Boxwala wrote: I think that’s a really clever way to increment toward splitting geode-core into more modules. I am excited to see what it looks like 👍 On Jun 20, 2019, at 7:45 PM, Jacob Barrett wrote: Gotcha! Sounds good. On Jun 20, 2019, at 4:35 PM, Dan Smith wrote: We don't have a membership gradle module, just a package. We're adding this to geode-core. For a little more context - we are thinking about refactoring membership (and/or maybe some other pieces) into separate gradle modules - proposal forthcoming! However, as a first step we need to untangle those pieces of code from the rest of geode-core. Rather than creating some long lived branch we can incrementally untangle the code a piece at a time, on develop. Having a way to track progress and enforce the direction of dependencies on the way to a separate gradle module will help with that. -Dan On Thu, Jun 20, 2019 at 4:23 PM Jacob Barrett wrote: Are you adding this dependency to just the membership module? I am cool with that. On Jun 20, 2019, at 2:39 PM, Dan Smith wrote: Hi all, Bill, Ernie, and I would like to add a new (apache licensed) test dependency to geode-core - https://github.com/TNG/ArchUnit. This is a tool that lets you write tests that make assertions about the interdependencies of your code - for example enforcing that package A does not depend on package B. Initially we intend to add some tests about what parts of the system the org.apache.geode.distributed.internal.membership package depends on, with an eye towards making that code more independently testable (proposal on that coming soon!). Does anyone have an issue with adding this test dependency? -Dan
Re: [DISCUSS] Add a test dependency to geode-core - ArchUnit
Two things come to mind: 1) uni-directional dependency Packages can be dependent on each other, because the classes inside of them can use each other. e.g.let’s say package A has class A1 and class A2 and package B has class B1 and B2. A1 can depend on B1, and B2 can depend on A2. Hence, the packages are dependent on each other. Modules can only have uni-directional dependency. If Module A depends on Module B, then no class in Module B can reference a class in Module A. This prevents tangling, i.e. spaghetti 2) Incremental compilation This lack of tangling helps not only developers, but the compiler too. In the packages example above, if I change any of the classes, all the code has to get recompiled because the dependency lines can go in any direction, and the compiler won’t attempt to optimize. In the modules case, if Module A changes, Module B will not recompile, because the dependency guarantees that nothing about Module B could have been affected. > On Jun 21, 2019, at 2:14 PM, Udo Kohlmeyer wrote: > > I know that I'm missing the benefit of physically moving the code from the > package into its own module. > > Could you possibly explain to me what it is? > > On 6/21/19 07:37, Murtuza Boxwala wrote: >> I think that’s a really clever way to increment toward splitting geode-core >> into more modules. I am excited to see what it looks like 👍 >> >>> On Jun 20, 2019, at 7:45 PM, Jacob Barrett wrote: >>> >>> Gotcha! Sounds good. >>> On Jun 20, 2019, at 4:35 PM, Dan Smith wrote: We don't have a membership gradle module, just a package. We're adding this to geode-core. For a little more context - we are thinking about refactoring membership (and/or maybe some other pieces) into separate gradle modules - proposal forthcoming! However, as a first step we need to untangle those pieces of code from the rest of geode-core. Rather than creating some long lived branch we can incrementally untangle the code a piece at a time, on develop. Having a way to track progress and enforce the direction of dependencies on the way to a separate gradle module will help with that. -Dan > On Thu, Jun 20, 2019 at 4:23 PM Jacob Barrett wrote: > > Are you adding this dependency to just the membership module? I am cool > with that. > >> On Jun 20, 2019, at 2:39 PM, Dan Smith wrote: >> >> Hi all, >> >> Bill, Ernie, and I would like to add a new (apache licensed) test >> dependency to geode-core - https://github.com/TNG/ArchUnit. This is a > tool >> that lets you write tests that make assertions about the > interdependencies >> of your code - for example enforcing that package A does not depend on >> package B. >> >> Initially we intend to add some tests about what parts of the system the >> org.apache.geode.distributed.internal.membership package depends on, with >> an eye towards making that code more independently testable (proposal on >> that coming soon!). >> >> Does anyone have an issue with adding this test dependency? >> >> -Dan >
Re: [DISCUSS] Add a test dependency to geode-core - ArchUnit
I know that I'm missing the benefit of physically moving the code from the package into its own module. Could you possibly explain to me what it is? On 6/21/19 07:37, Murtuza Boxwala wrote: I think that’s a really clever way to increment toward splitting geode-core into more modules. I am excited to see what it looks like 👍 On Jun 20, 2019, at 7:45 PM, Jacob Barrett wrote: Gotcha! Sounds good. On Jun 20, 2019, at 4:35 PM, Dan Smith wrote: We don't have a membership gradle module, just a package. We're adding this to geode-core. For a little more context - we are thinking about refactoring membership (and/or maybe some other pieces) into separate gradle modules - proposal forthcoming! However, as a first step we need to untangle those pieces of code from the rest of geode-core. Rather than creating some long lived branch we can incrementally untangle the code a piece at a time, on develop. Having a way to track progress and enforce the direction of dependencies on the way to a separate gradle module will help with that. -Dan On Thu, Jun 20, 2019 at 4:23 PM Jacob Barrett wrote: Are you adding this dependency to just the membership module? I am cool with that. On Jun 20, 2019, at 2:39 PM, Dan Smith wrote: Hi all, Bill, Ernie, and I would like to add a new (apache licensed) test dependency to geode-core - https://github.com/TNG/ArchUnit. This is a tool that lets you write tests that make assertions about the interdependencies of your code - for example enforcing that package A does not depend on package B. Initially we intend to add some tests about what parts of the system the org.apache.geode.distributed.internal.membership package depends on, with an eye towards making that code more independently testable (proposal on that coming soon!). Does anyone have an issue with adding this test dependency? -Dan
Fixed develop
I briefly broke compile on develop with a bungled PR merge. I broke a constructor in DistributionStats because of a conflict during merge. It's fixed now.
Please review PRs
We need some additional PR reviewers on the following PRs. I've tagged who I think should review them but it's just a suggestion (or guess in some cases). Please feel free to review anything you're not tagged on as well. Some of these just need to be *re*-reviewed after requested changes have been made. GEODE-5557: Fix for false read conflict exception #3492 https://github.com/apache/geode/pull/3492 GEODE-2863: Discard all new tasks, after close request received #3624 https://github.com/apache/geode/pull/3624 GEODE-3718: InternalResourceManager thread pool shutdown improvement #3655 https://github.com/apache/geode/pull/3655 GEODE-6870: Solution for creation of non-persistent overflow region #3724 https://github.com/apache/geode/pull/3724 I've reviewed some of these but lack expertise in a couple of the areas and it would be best to get additional reviewers. PS: I purposely left off this list any PRs that currently have conflicts with develop or still have what appears to be unaddressed feedback/reviews. Thanks, Kirk
Re: [PROPOSAL]: Improve OQL Method Invocation Security
Hello Dan, Thanks a lot for the feedback!. Please find my answers below. (1). Great point, I totally missed this one... I wrongly assumed that *this particular method [1]* was in charge of authorizing the query execution on the region but I was clearly wrong, the actual authorization happens *here [2]. *The former just kicks in whenever a method is executed on the region itself (like *getValues*, *keySet*, etc.). (2). Agreed, another oversight on my side, mostly derived from my misunderstanding in item 1. 3). I'm not sure about how this is going to work either, neither if it's even possible without hugely impacting the performance... truth be told, the whole idea of this particular authorizer is to allow users to get up and running *without configuring anything, *but I'm not sure how it can be done in the real world (specially now that you made me aware my error on item 1). I'd actually love to just eliminate this authorizer altogether but it was brought to my attention that the other two proposed out of box implementations require the user to actually configure something (the package or the regex expression), and that we should provide some sort of automatic trust boundary in the sense that users just deploy the domain model, insert objects into the region, and can execute OQL invoking methods as long as those methods belong to the objects stored within the region... determining whether an object belongs to a region or not is fairly complicated, not achievable right now and will certainly incur into performance degradation, but I guess is up to the community to decide whether we should pursue this objective or not. As a summary, and before I go ahead and incorporate all the feedback provided and modify the proposal, I guess the community needs to answer the following questions: A. For the sake of security, is it okay to force our users to do some extra configuration steps (no matter how little or easy they might be) whenever they deploy the domain model to the cluster and want to execute some methods on those objects from OQL?. B. Should we just change the approach from *deny everything* to *allow everything *instead, and assume that users with enough privileges to execute queries on the region (*DATA:READ:RegionName*) are trustworthy (at the end of the day *somebody* gave them that role) and won't compromise the security through RCE from OQL?. If nothing changes during the weekend, I'll wait until Monday and apply the suggested changes to the proposal in favour of a "YES" to question "A" (just because that's my preference at the moment), sending a new email afterward for another feedback round. Changes will still be accepted anyway as this is just the proposal and the community as whole should deny/accept it. Cheers. [1]: https://github.com/apache/geode/blob/develop/geode-core/src/main/java/org/apache/geode/cache/query/internal/RestrictedMethodInvocationAuthorizer.java#L165-L171 [2]: https://github.com/apache/geode/blob/develop/geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/BaseCommandQuery.java#L106-L108 On Thu, Jun 20, 2019 at 7:22 PM Dan Smith wrote: > +1 > > This looks really good! > > I put a couple of comments inline, and I have a few more general questions > here: > 1. Is the RegionQueryInvocationAuthorizer different than our existing shiro > permissions? I thought users can already grant permissions for specific > regions. What does this add in addition to that? > 2. I'm a little unclear on if your > MethodInvocationAuthorizer.authorizeMethodInvocation is supposed to take a > region or the target object. If it is really accepting a region, do we > actually have a region available in all cases? We could be invoking methods > on an object in lots of places in the query tree. > 3. The DataAwareBasedMethodAuthorizer seems a bit vague on how it's > actually going to work. It also might be a security risk, since it will > allow users with DATA:READ permission to invoke any method on these > objects. > > -Dan > > On Wed, Jun 19, 2019 at 11:34 AM Jacob Barrett > wrote: > > > Thanks Juan! > > > > > On Jun 19, 2019, at 9:55 AM, Juan José Ramos > wrote: > > > > > > Hello all, > > > > > > I've removed all "biased" words I could find from the original document > > so > > > the *Proposal [1]* is ready for review and discussion now. All feedback > > is > > > welcome. > > > Best regards. > > > > > > [1]: > > > > > > https://cwiki.apache.org/confluence/display/GEODE/OQL+Method+Invocation+Security > > > > > >> On Fri, Jun 14, 2019 at 8:39 PM Juan José Ramos > > wrote: > > >> > > >> Hey Jake, > > >> > > >> Thanks for bringing this up. As you might have found out already, > > english > > >> is not my native language, I actually had to do some research to find > > out > > >> *exactly what you meant* regarding the bias around the "whitelist" > word > > >> :-|... It was an honest mistake and I sincerely apologize in advance > if > > >> anyone got offended in any way. > > >> That sa
Re: [DISCUSS] Add a test dependency to geode-core - ArchUnit
I think that’s a really clever way to increment toward splitting geode-core into more modules. I am excited to see what it looks like 👍 > On Jun 20, 2019, at 7:45 PM, Jacob Barrett wrote: > > Gotcha! Sounds good. > >> On Jun 20, 2019, at 4:35 PM, Dan Smith wrote: >> >> We don't have a membership gradle module, just a package. We're adding this >> to geode-core. >> >> For a little more context - we are thinking about refactoring membership >> (and/or maybe some other pieces) into separate gradle modules - proposal >> forthcoming! However, as a first step we need to untangle those pieces of >> code from the rest of geode-core. Rather than creating some long lived >> branch we can incrementally untangle the code a piece at a time, on >> develop. Having a way to track progress and enforce the direction of >> dependencies on the way to a separate gradle module will help with that. >> >> -Dan >> >>> On Thu, Jun 20, 2019 at 4:23 PM Jacob Barrett wrote: >>> >>> Are you adding this dependency to just the membership module? I am cool >>> with that. >>> On Jun 20, 2019, at 2:39 PM, Dan Smith wrote: Hi all, Bill, Ernie, and I would like to add a new (apache licensed) test dependency to geode-core - https://github.com/TNG/ArchUnit. This is a >>> tool that lets you write tests that make assertions about the >>> interdependencies of your code - for example enforcing that package A does not depend on package B. Initially we intend to add some tests about what parts of the system the org.apache.geode.distributed.internal.membership package depends on, with an eye towards making that code more independently testable (proposal on that coming soon!). Does anyone have an issue with adding this test dependency? -Dan >>> >>>
