> On Sept. 19, 2022, 1:24 a.m., Abhay Kulkarni wrote: > > agents-common/src/main/java/org/apache/ranger/plugin/util/RangerPolicyDeltaUtil.java > > Line 113 (original), 108 (patched) > > <https://reviews.apache.org/r/74112/diff/1/?file=2269952#file2269952line114> > > > > For a deleted policy, there will always be exactly one element in > > deletedPolicies array. Therefore, the error condition is if that is not > > true. > > > > Please review this change, and provide a test case where the existing > > condition is shown to be incorrect. > > Ramachandran Krishnan wrote: > Thanks for reviewing it .I did not encounter the issue where > deletedPolicies is more than one .It is very much possible that record will > be more than one when compressDelta does not work correctly.In this case > whatever the code is present is make sense .Let me close this review .I will > do some testing .If i encounter any issue , i will create the patch for that > issue
Abhay Kulkarni Could you please take a look into this ? - Ramachandran ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/74112/#review224677 ----------------------------------------------------------- On Nov. 28, 2022, 9:53 a.m., Ramachandran Krishnan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/74112/ > ----------------------------------------------------------- > > (Updated Nov. 28, 2022, 9:53 a.m.) > > > Review request for ranger, Don Bosco Durai, 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; > } > From the above code, we iterate delta policies and check if this policy > exists in the existing policy, we add that to deletePolicies list. > > If a delta change type is created, we expect that it should not be in the > existing old policy which should not have happened. So the below block code > is not needed > > 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; > } > > > Diffs > ----- > > > agents-common/src/main/java/org/apache/ranger/plugin/util/RangerPolicyDeltaUtil.java > e9223fe69 > > > Diff: https://reviews.apache.org/r/74112/diff/3/ > > > 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 > >
