Re: Review Request 57987: RANGER-1478 : Small refactor in RangerPolicyEngineCache and RangerPolicyEngineOptions, to avoid looking up RangerConfiguration everytime, and try to write the RPEO fields only from that class (OOP)

[Zsombor Gegesy <gz...@gmail.com>]

> On March 31, 2017, 3:31 p.m., Colm O hEigeartaigh wrote:
> > I'm wondering if it makes sense to make the default getPolicyEngineOptions() configured via "configureDelegateAdmin"? Why not just let ServiceREST do that part?

I thought about creating a constructor with all the fields, and using that in various places, but it seemed a bit harder to follow the logic with one string field, and 6 booleans in the arguments of the constructor.


- Zsombor


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


On March 31, 2017, 12:50 p.m., Zsombor Gegesy wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57987/
> -----------------------------------------------------------
> 
> (Updated March 31, 2017, 12:50 p.m.)
> 
> 
> Review request for ranger.
> 
> 
> Bugs: RANGER-1478
>     https://issues.apache.org/jira/browse/RANGER-1478
> 
> 
> Repository: ranger
> 
> 
> Description
> -------
> 
> RangerPolicyEngineOptions has a lot of public fields, which is written from various places from the code base, which should be avoided. That object is configured from RangerConfiguration, but in the middle of the plugin initialization code, which makes this a bit more complex, than it should be.
> Suggestions:
> 
>     RangerConfiguration should be treated as an object, not a static facade for a couple of config values
>     RangerPolicyEngineOptions should get his configuration from directly the RangerConfiguration, in an explicit, encapsulated way.
> 
> 
> Diffs
> -----
> 
>   agents-common/src/main/java/org/apache/ranger/plugin/policyengine/RangerPolicyEngineCache.java 5376b52 
>   agents-common/src/main/java/org/apache/ranger/plugin/policyengine/RangerPolicyEngineOptions.java a9027bc 
>   agents-common/src/main/java/org/apache/ranger/plugin/service/RangerBasePlugin.java acf8d15 
>   security-admin/src/main/java/org/apache/ranger/rest/ServiceREST.java 6176319 
> 
> 
> Diff: https://reviews.apache.org/r/57987/diff/2/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Zsombor Gegesy
> 
>