----------------------------------------------------------- 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 > >
