You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ranger.apache.org by Abhishek Kumar <ab...@gmail.com> on 2023/01/24 08:28:57 UTC
Re: Review Request 74112: RANGER-3903:Improvement in RangerPolicyDeltaUtil--> applyDeltas method
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/74112/#review225121
-----------------------------------------------------------
agents-common/src/main/java/org/apache/ranger/plugin/util/RangerPolicyDeltaUtil.java
Line 61 (original), 63 (patched)
<https://reviews.apache.org/r/74112/#comment313905>
This for loop may not be required, serviceType check can be incorporated in the for block beginning line no 88.
agents-common/src/main/java/org/apache/ranger/plugin/util/RangerPolicyDeltaUtil.java
Line 89 (original), 104 (patched)
<https://reviews.apache.org/r/74112/#comment313904>
addAll may be avoided here. How about deletedPolicies = policiesIdMap.get(policyId); ?
- Abhishek Kumar
On Jan. 4, 2023, 7:12 a.m., Ramachandran Krishnan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/74112/
> -----------------------------------------------------------
>
> (Updated Jan. 4, 2023, 7:12 a.m.)
>
>
> Review request for ranger, Don Bosco Durai, Kirby Zhou, Abhay Kulkarni, Madhan Neethiraj, Mehul Parikh, Nikhil P, Pradeep Agrawal, Ramesh Mani, Selvamohan Neethiraj, Sailaja Polavarapu, Subhrat Chaudhary, and Velmurugan Periasamy.
>
>
> Bugs: RANGER-3903
> https://issues.apache.org/jira/browse/RANGER-3903
>
>
> Repository: ranger
>
>
> Description
> -------
>
> After going through the below code snippets in the master branch
>
> while (iter.hasNext()) {
> RangerPolicy policy = iter.next();
> if (policyId.equals(policy.getId()) && (changeType == RangerPolicyDelta.CHANGE_TYPE_POLICY_DELETE || changeType == RangerPolicyDelta.CHANGE_TYPE_POLICY_UPDATE)) {
> deletedPolicies.add(policy);
> iter.remove();
> }
> }
> switch (changeType) {
> case RangerPolicyDelta.CHANGE_TYPE_POLICY_CREATE:
> {
> if (CollectionUtils.isNotEmpty(deletedPolicies)) {
> LOG.warn("Unexpected: found existing policy for CHANGE_TYPE_POLICY_CREATE: " + Arrays.toString(deletedPolicies.toArray()));
> }
> break;
> }
> case RangerPolicyDelta.CHANGE_TYPE_POLICY_UPDATE:
> {
> if (CollectionUtils.isEmpty(deletedPolicies) || deletedPolicies.size() > 1) {
> LOG.warn("Unexpected: found no policy or multiple policies for CHANGE_TYPE_POLICY_UPDATE: " + Arrays.toString(deletedPolicies.toArray()));
> }
> break;
> }
> case RangerPolicyDelta.CHANGE_TYPE_POLICY_DELETE:
> {
> if (CollectionUtils.isEmpty(deletedPolicies) || deletedPolicies.size() > 1) {
> LOG.warn("Unexpected: found no policy or multiple policies for CHANGE_TYPE_POLICY_DELETE: " + Arrays.toString(deletedPolicies.toArray()));
> }
> break;
> }
> default:
> break;
> }
>
> 1st#improvement:
>
> From the above code, we iterate delta policies and check if this policy exists in the existing policy, we add that to deletePolicies list.
>
> The delta change type condition for created/updated/deleted is added on top of the if the condition so adding the condition again is not necessary
>
> 2nd#improvement:
> From the above code, we see for each element in the deltas,we iterate policies and check if this delta policy exists in the existing policy, we add that to deletePolicies list.
>
> Solution:
> We need to use Map instead of iterating policies for every element of deltas --> Map<Long,List<RangerPolicy>> policiesIdMap
> The building index map key will be policyId and the value will be no of policies associated with the same policy Id
>
>
> For each policy in the deltas , we check in the policiesIdMap whether the same policyId is present or not ?.
>
>
> if yes ,we will add all the associated policy list into deletePolicies and remove the policyId from policiesIdMap
>
>
> After end of the iteration,we will iterate the policiesIdMap and get all the policylist associated with policyId and add into the result set
> This will give better performance when the policies list is huge.
>
>
> Diffs
> -----
>
> agents-common/src/main/java/org/apache/ranger/plugin/util/RangerPolicyDeltaUtil.java e9223fe69
>
>
> Diff: https://reviews.apache.org/r/74112/diff/5/
>
>
> Testing
> -------
>
> Tested the below Rest API's to make sure everything works fine
>
> 1. ServiceREST Rest API :GET /plugins/policies/download/{serviceName}
>
> 2. ServiceREST Rest API :GET /plugins/secure/policies/download/{serviceName}
>
>
> Thanks,
>
> Ramachandran Krishnan
>
>
Re: Review Request 74112: RANGER-3903:Improvement in RangerPolicyDeltaUtil--> applyDeltas method
Posted by Ramachandran Krishnan <ra...@gmail.com>.
> On Jan. 24, 2023, 8:28 a.m., Abhishek Kumar wrote:
> > agents-common/src/main/java/org/apache/ranger/plugin/util/RangerPolicyDeltaUtil.java
> > Line 61 (original), 63 (patched)
> > <https://reviews.apache.org/r/74112/diff/5/?file=2272594#file2272594line63>
> >
> > This for loop may not be required, serviceType check can be incorporated in the for block beginning line no 88.
>
> Ramachandran Krishnan wrote:
> Can you please elobrate a bit more about this review comment
>
> Abhishek Kumar wrote:
> It's essentially a code refactor to get rid of the below block
> // to be deleted
> //for (RangerPolicyDelta delta : deltas) {
> // if (serviceType.equals(delta.getServiceType())) {
> // hasExpectedServiceType = true;
> // break;
> // } else if (!serviceType.equals(EmbeddedServiceDefsUtil.EMBEDDED_SERVICEDEF_TAG_NAME) && !delta.getServiceType().equals(EmbeddedServiceDefsUtil.EMBEDDED_SERVICEDEF_TAG_NAME)) {
> // LOG.warn("Found unexpected serviceType in policyDelta:[" + delta + "]. Was expecting serviceType:[" + serviceType + "]. Should NOT have come here!! Ignoring delta and continuing");
> // }
> //}
>
> and modified to below:
>
> for (RangerPolicyDelta delta : deltas) { // from line 88 in the new patch
> if (serviceType.equals(delta.getServiceType())) {
> if (policyId == null) {
> continue;
> }
>
> List<RangerPolicy> deletedPolicies = new ArrayList<>();
> if (policiesIdMap.containsKey(policyId)) {
> deletedPolicies = policiesIdMap.get(policyId);
> policiesIdMap.remove(policyId);
>
> switch
>
> ...
> ...
> ...
>
> } else {
> // LOG.warn("Found unexpected serviceType in policyDelta:[" + delta + "]. Was expecting serviceType:[" + serviceType + "]. Should NOT have come here!! Ignoring delta and continuing");
> }
> }
>
> Thanks!
It is a good suggestion to implement . I will address it
- Ramachandran
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/74112/#review225121
-----------------------------------------------------------
On Jan. 24, 2023, 4:06 p.m., Ramachandran Krishnan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/74112/
> -----------------------------------------------------------
>
> (Updated Jan. 24, 2023, 4:06 p.m.)
>
>
> Review request for ranger, Don Bosco Durai, Kirby Zhou, Abhay Kulkarni, Madhan Neethiraj, Mehul Parikh, Nikhil P, Pradeep Agrawal, Ramesh Mani, Selvamohan Neethiraj, Sailaja Polavarapu, Subhrat Chaudhary, and Velmurugan Periasamy.
>
>
> Bugs: RANGER-3903
> https://issues.apache.org/jira/browse/RANGER-3903
>
>
> Repository: ranger
>
>
> Description
> -------
>
> After going through the below code snippets in the master branch
>
> while (iter.hasNext()) {
> RangerPolicy policy = iter.next();
> if (policyId.equals(policy.getId()) && (changeType == RangerPolicyDelta.CHANGE_TYPE_POLICY_DELETE || changeType == RangerPolicyDelta.CHANGE_TYPE_POLICY_UPDATE)) {
> deletedPolicies.add(policy);
> iter.remove();
> }
> }
> switch (changeType) {
> case RangerPolicyDelta.CHANGE_TYPE_POLICY_CREATE:
> {
> if (CollectionUtils.isNotEmpty(deletedPolicies)) {
> LOG.warn("Unexpected: found existing policy for CHANGE_TYPE_POLICY_CREATE: " + Arrays.toString(deletedPolicies.toArray()));
> }
> break;
> }
> case RangerPolicyDelta.CHANGE_TYPE_POLICY_UPDATE:
> {
> if (CollectionUtils.isEmpty(deletedPolicies) || deletedPolicies.size() > 1) {
> LOG.warn("Unexpected: found no policy or multiple policies for CHANGE_TYPE_POLICY_UPDATE: " + Arrays.toString(deletedPolicies.toArray()));
> }
> break;
> }
> case RangerPolicyDelta.CHANGE_TYPE_POLICY_DELETE:
> {
> if (CollectionUtils.isEmpty(deletedPolicies) || deletedPolicies.size() > 1) {
> LOG.warn("Unexpected: found no policy or multiple policies for CHANGE_TYPE_POLICY_DELETE: " + Arrays.toString(deletedPolicies.toArray()));
> }
> break;
> }
> default:
> break;
> }
>
> 1st#improvement:
>
> From the above code, we iterate delta policies and check if this policy exists in the existing policy, we add that to deletePolicies list.
>
> The delta change type condition for created/updated/deleted is added on top of the if the condition so adding the condition again is not necessary
>
> 2nd#improvement:
> From the above code, we see for each element in the deltas,we iterate policies and check if this delta policy exists in the existing policy, we add that to deletePolicies list.
>
> Solution:
> We need to use Map instead of iterating policies for every element of deltas --> Map<Long,List<RangerPolicy>> policiesIdMap
> The building index map key will be policyId and the value will be no of policies associated with the same policy Id
>
>
> For each policy in the deltas , we check in the policiesIdMap whether the same policyId is present or not ?.
>
>
> if yes ,we will add all the associated policy list into deletePolicies and remove the policyId from policiesIdMap
>
>
> After end of the iteration,we will iterate the policiesIdMap and get all the policylist associated with policyId and add into the result set
> This will give better performance when the policies list is huge.
>
>
> Diffs
> -----
>
> agents-common/src/main/java/org/apache/ranger/plugin/util/RangerPolicyDeltaUtil.java e9223fe69
>
>
> Diff: https://reviews.apache.org/r/74112/diff/6/
>
>
> Testing
> -------
>
> Tested the below Rest API's to make sure everything works fine
>
> 1. ServiceREST Rest API :GET /plugins/policies/download/{serviceName}
>
> 2. ServiceREST Rest API :GET /plugins/secure/policies/download/{serviceName}
>
>
> Thanks,
>
> Ramachandran Krishnan
>
>
Re: Review Request 74112: RANGER-3903:Improvement in RangerPolicyDeltaUtil--> applyDeltas method
Posted by Abhishek Kumar <ab...@gmail.com>.
> On Jan. 24, 2023, 8:28 a.m., Abhishek Kumar wrote:
> > agents-common/src/main/java/org/apache/ranger/plugin/util/RangerPolicyDeltaUtil.java
> > Line 61 (original), 63 (patched)
> > <https://reviews.apache.org/r/74112/diff/5/?file=2272594#file2272594line63>
> >
> > This for loop may not be required, serviceType check can be incorporated in the for block beginning line no 88.
>
> Ramachandran Krishnan wrote:
> Can you please elobrate a bit more about this review comment
It's essentially a code refactor to get rid of the below block
// to be deleted
//for (RangerPolicyDelta delta : deltas) {
// if (serviceType.equals(delta.getServiceType())) {
// hasExpectedServiceType = true;
// break;
// } else if (!serviceType.equals(EmbeddedServiceDefsUtil.EMBEDDED_SERVICEDEF_TAG_NAME) && !delta.getServiceType().equals(EmbeddedServiceDefsUtil.EMBEDDED_SERVICEDEF_TAG_NAME)) {
// LOG.warn("Found unexpected serviceType in policyDelta:[" + delta + "]. Was expecting serviceType:[" + serviceType + "]. Should NOT have come here!! Ignoring delta and continuing");
// }
//}
and modified to below:
for (RangerPolicyDelta delta : deltas) { // from line 88 in the new patch
if (serviceType.equals(delta.getServiceType())) {
if (policyId == null) {
continue;
}
List<RangerPolicy> deletedPolicies = new ArrayList<>();
if (policiesIdMap.containsKey(policyId)) {
deletedPolicies = policiesIdMap.get(policyId);
policiesIdMap.remove(policyId);
switch
...
...
...
} else {
// LOG.warn("Found unexpected serviceType in policyDelta:[" + delta + "]. Was expecting serviceType:[" + serviceType + "]. Should NOT have come here!! Ignoring delta and continuing");
}
}
Thanks!
- Abhishek
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/74112/#review225121
-----------------------------------------------------------
On Jan. 24, 2023, 4:06 p.m., Ramachandran Krishnan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/74112/
> -----------------------------------------------------------
>
> (Updated Jan. 24, 2023, 4:06 p.m.)
>
>
> Review request for ranger, Don Bosco Durai, Kirby Zhou, Abhay Kulkarni, Madhan Neethiraj, Mehul Parikh, Nikhil P, Pradeep Agrawal, Ramesh Mani, Selvamohan Neethiraj, Sailaja Polavarapu, Subhrat Chaudhary, and Velmurugan Periasamy.
>
>
> Bugs: RANGER-3903
> https://issues.apache.org/jira/browse/RANGER-3903
>
>
> Repository: ranger
>
>
> Description
> -------
>
> After going through the below code snippets in the master branch
>
> while (iter.hasNext()) {
> RangerPolicy policy = iter.next();
> if (policyId.equals(policy.getId()) && (changeType == RangerPolicyDelta.CHANGE_TYPE_POLICY_DELETE || changeType == RangerPolicyDelta.CHANGE_TYPE_POLICY_UPDATE)) {
> deletedPolicies.add(policy);
> iter.remove();
> }
> }
> switch (changeType) {
> case RangerPolicyDelta.CHANGE_TYPE_POLICY_CREATE:
> {
> if (CollectionUtils.isNotEmpty(deletedPolicies)) {
> LOG.warn("Unexpected: found existing policy for CHANGE_TYPE_POLICY_CREATE: " + Arrays.toString(deletedPolicies.toArray()));
> }
> break;
> }
> case RangerPolicyDelta.CHANGE_TYPE_POLICY_UPDATE:
> {
> if (CollectionUtils.isEmpty(deletedPolicies) || deletedPolicies.size() > 1) {
> LOG.warn("Unexpected: found no policy or multiple policies for CHANGE_TYPE_POLICY_UPDATE: " + Arrays.toString(deletedPolicies.toArray()));
> }
> break;
> }
> case RangerPolicyDelta.CHANGE_TYPE_POLICY_DELETE:
> {
> if (CollectionUtils.isEmpty(deletedPolicies) || deletedPolicies.size() > 1) {
> LOG.warn("Unexpected: found no policy or multiple policies for CHANGE_TYPE_POLICY_DELETE: " + Arrays.toString(deletedPolicies.toArray()));
> }
> break;
> }
> default:
> break;
> }
>
> 1st#improvement:
>
> From the above code, we iterate delta policies and check if this policy exists in the existing policy, we add that to deletePolicies list.
>
> The delta change type condition for created/updated/deleted is added on top of the if the condition so adding the condition again is not necessary
>
> 2nd#improvement:
> From the above code, we see for each element in the deltas,we iterate policies and check if this delta policy exists in the existing policy, we add that to deletePolicies list.
>
> Solution:
> We need to use Map instead of iterating policies for every element of deltas --> Map<Long,List<RangerPolicy>> policiesIdMap
> The building index map key will be policyId and the value will be no of policies associated with the same policy Id
>
>
> For each policy in the deltas , we check in the policiesIdMap whether the same policyId is present or not ?.
>
>
> if yes ,we will add all the associated policy list into deletePolicies and remove the policyId from policiesIdMap
>
>
> After end of the iteration,we will iterate the policiesIdMap and get all the policylist associated with policyId and add into the result set
> This will give better performance when the policies list is huge.
>
>
> Diffs
> -----
>
> agents-common/src/main/java/org/apache/ranger/plugin/util/RangerPolicyDeltaUtil.java e9223fe69
>
>
> Diff: https://reviews.apache.org/r/74112/diff/6/
>
>
> Testing
> -------
>
> Tested the below Rest API's to make sure everything works fine
>
> 1. ServiceREST Rest API :GET /plugins/policies/download/{serviceName}
>
> 2. ServiceREST Rest API :GET /plugins/secure/policies/download/{serviceName}
>
>
> Thanks,
>
> Ramachandran Krishnan
>
>
Re: Review Request 74112: RANGER-3903:Improvement in RangerPolicyDeltaUtil--> applyDeltas method
Posted by Ramachandran Krishnan <ra...@gmail.com>.
> On Jan. 24, 2023, 8:28 a.m., Abhishek Kumar wrote:
> > agents-common/src/main/java/org/apache/ranger/plugin/util/RangerPolicyDeltaUtil.java
> > Line 61 (original), 63 (patched)
> > <https://reviews.apache.org/r/74112/diff/5/?file=2272594#file2272594line63>
> >
> > This for loop may not be required, serviceType check can be incorporated in the for block beginning line no 88.
Can you please elobrate a bit more about this review comment
- Ramachandran
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/74112/#review225121
-----------------------------------------------------------
On Jan. 24, 2023, 4:06 p.m., Ramachandran Krishnan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/74112/
> -----------------------------------------------------------
>
> (Updated Jan. 24, 2023, 4:06 p.m.)
>
>
> Review request for ranger, Don Bosco Durai, Kirby Zhou, Abhay Kulkarni, Madhan Neethiraj, Mehul Parikh, Nikhil P, Pradeep Agrawal, Ramesh Mani, Selvamohan Neethiraj, Sailaja Polavarapu, Subhrat Chaudhary, and Velmurugan Periasamy.
>
>
> Bugs: RANGER-3903
> https://issues.apache.org/jira/browse/RANGER-3903
>
>
> Repository: ranger
>
>
> Description
> -------
>
> After going through the below code snippets in the master branch
>
> while (iter.hasNext()) {
> RangerPolicy policy = iter.next();
> if (policyId.equals(policy.getId()) && (changeType == RangerPolicyDelta.CHANGE_TYPE_POLICY_DELETE || changeType == RangerPolicyDelta.CHANGE_TYPE_POLICY_UPDATE)) {
> deletedPolicies.add(policy);
> iter.remove();
> }
> }
> switch (changeType) {
> case RangerPolicyDelta.CHANGE_TYPE_POLICY_CREATE:
> {
> if (CollectionUtils.isNotEmpty(deletedPolicies)) {
> LOG.warn("Unexpected: found existing policy for CHANGE_TYPE_POLICY_CREATE: " + Arrays.toString(deletedPolicies.toArray()));
> }
> break;
> }
> case RangerPolicyDelta.CHANGE_TYPE_POLICY_UPDATE:
> {
> if (CollectionUtils.isEmpty(deletedPolicies) || deletedPolicies.size() > 1) {
> LOG.warn("Unexpected: found no policy or multiple policies for CHANGE_TYPE_POLICY_UPDATE: " + Arrays.toString(deletedPolicies.toArray()));
> }
> break;
> }
> case RangerPolicyDelta.CHANGE_TYPE_POLICY_DELETE:
> {
> if (CollectionUtils.isEmpty(deletedPolicies) || deletedPolicies.size() > 1) {
> LOG.warn("Unexpected: found no policy or multiple policies for CHANGE_TYPE_POLICY_DELETE: " + Arrays.toString(deletedPolicies.toArray()));
> }
> break;
> }
> default:
> break;
> }
>
> 1st#improvement:
>
> From the above code, we iterate delta policies and check if this policy exists in the existing policy, we add that to deletePolicies list.
>
> The delta change type condition for created/updated/deleted is added on top of the if the condition so adding the condition again is not necessary
>
> 2nd#improvement:
> From the above code, we see for each element in the deltas,we iterate policies and check if this delta policy exists in the existing policy, we add that to deletePolicies list.
>
> Solution:
> We need to use Map instead of iterating policies for every element of deltas --> Map<Long,List<RangerPolicy>> policiesIdMap
> The building index map key will be policyId and the value will be no of policies associated with the same policy Id
>
>
> For each policy in the deltas , we check in the policiesIdMap whether the same policyId is present or not ?.
>
>
> if yes ,we will add all the associated policy list into deletePolicies and remove the policyId from policiesIdMap
>
>
> After end of the iteration,we will iterate the policiesIdMap and get all the policylist associated with policyId and add into the result set
> This will give better performance when the policies list is huge.
>
>
> Diffs
> -----
>
> agents-common/src/main/java/org/apache/ranger/plugin/util/RangerPolicyDeltaUtil.java e9223fe69
>
>
> Diff: https://reviews.apache.org/r/74112/diff/6/
>
>
> Testing
> -------
>
> Tested the below Rest API's to make sure everything works fine
>
> 1. ServiceREST Rest API :GET /plugins/policies/download/{serviceName}
>
> 2. ServiceREST Rest API :GET /plugins/secure/policies/download/{serviceName}
>
>
> Thanks,
>
> Ramachandran Krishnan
>
>