----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/74112/#review225134 -----------------------------------------------------------
agents-common/src/main/java/org/apache/ranger/plugin/util/RangerPolicyDeltaUtil.java Line 62 (original), 64 (patched) <https://reviews.apache.org/r/74112/#comment313916> Although the comment explains how it affects the change, I believe it won't serve any purpose once the code gets merged. It would be good to have this comment as part of this review instead. agents-common/src/main/java/org/apache/ranger/plugin/util/RangerPolicyDeltaUtil.java Lines 70 (patched) <https://reviews.apache.org/r/74112/#comment313913> There are no duplicates in the input param List<RangerPolicy> policies so the Map<Long, List<RangerPolicy>> should be Map<Long, RangerPolicy> instead. agents-common/src/main/java/org/apache/ranger/plugin/util/RangerPolicyDeltaUtil.java Lines 72 (patched) <https://reviews.apache.org/r/74112/#comment313920> policy.getId() is called 3 times here, assigning it to a variable is suggested here. agents-common/src/main/java/org/apache/ranger/plugin/util/RangerPolicyDeltaUtil.java Line 75 (original), 80 (patched) <https://reviews.apache.org/r/74112/#comment313915> A better name here would be hasAtLeastOneExpectedServiceType agents-common/src/main/java/org/apache/ranger/plugin/util/RangerPolicyDeltaUtil.java Line 87 (original), 90 (patched) <https://reviews.apache.org/r/74112/#comment313914> List<RangerPolicy> deletedPolicies = new ArrayList<>(); should be RangerPolicy deletedPolicy; instead. agents-common/src/main/java/org/apache/ranger/plugin/util/RangerPolicyDeltaUtil.java Line 101 (original), 98 (patched) <https://reviews.apache.org/r/74112/#comment313917> Null check on the deleted policy should suffice and CollectionUtils may be avoided. agents-common/src/main/java/org/apache/ranger/plugin/util/RangerPolicyDeltaUtil.java Line 107 (original), 104 (patched) <https://reviews.apache.org/r/74112/#comment313918> size check can be avoided. agents-common/src/main/java/org/apache/ranger/plugin/util/RangerPolicyDeltaUtil.java Line 113 (original), 110 (patched) <https://reviews.apache.org/r/74112/#comment313919> size check can be avoided. - Abhishek Kumar On Jan. 25, 2023, 4:50 a.m., Ramachandran Krishnan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/74112/ > ----------------------------------------------------------- > > (Updated Jan. 25, 2023, 4:50 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/7/ > > > 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 > >
