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