You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ranger.apache.org by "mneethiraj (via GitHub)" <gi...@apache.org> on 2025/09/16 20:20:22 UTC
[PR] RANGER-5324: replace iterations with streams in RangerRequestScriptEvaluator [ranger]
mneethiraj opened a new pull request, #671:
URL: https://github.com/apache/ranger/pull/671
## What changes were proposed in this pull request?
Replaced iterations with streams equivalents in RangerRequestScriptEvaluator
## How was this patch tested?
All tests pass sucessfully.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@ranger.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RANGER-5324: replace iterations with streams in RangerRequestScriptEvaluator [ranger]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #671:
URL: https://github.com/apache/ranger/pull/671#discussion_r2353565978
##########
agents-common/src/main/java/org/apache/ranger/plugin/policyengine/RangerRequestScriptEvaluator.java:
##########
@@ -365,7 +354,7 @@ public String getResourceZone() {
public Set<String> getResourceZones() {
Set<String> ret = RangerAccessRequestUtil.getResourceZoneNamesFromContext(getRequestContext());
- return ret != null ? Collections.emptySet() : ret;
+ return ret != null ? ret : Collections.emptySet();
Review Comment:
The null check logic is inverted. When ret is null, you should return Collections.emptySet(), but currently you're returning ret (which is null) when ret is not null.
##########
agents-common/src/main/java/org/apache/ranger/plugin/policyengine/RangerRequestScriptEvaluator.java:
##########
@@ -1160,124 +1030,75 @@ private Map<String, String> copyMap(Map<String, String> obj) {
}
private List<Object> getUgAttr(String attrName) {
- List<Object> ret = new ArrayList<>();
-
- for (String groupName : userGroups) {
- Map<String, String> attrs = groupAttrs.get(groupName);
- Object val = attrs != null ? attrs.get(attrName) : null;
-
- if (val != null) {
- ret.add(val);
- }
- }
-
- return ret;
+ return userGroups.stream()
+ .map(groupAttrs::get)
+ .filter(Objects::nonNull)
+ .map(attrs -> attrs.get(attrName))
+ .filter(Objects::nonNull)
+ .collect(Collectors.toList());
}
private List<Object> getTagAttr(String attrName) {
- List<Object> ret = new ArrayList<>();
-
- for (String tagName : tagNames) {
- Map<String, Object> attrs = tags.get(tagName);
- Object val = attrs != null ? attrs.get(attrName) : null;
-
- if (val != null) {
- ret.add(val);
- }
- }
-
- return ret;
+ return tagNames.stream()
+ .map(tags::get)
+ .filter(Objects::nonNull)
+ .map(attrs -> attrs.get(attrName))
+ .filter(Objects::nonNull)
+ .collect(Collectors.toList());
}
private Collection<String> getUserAttrNames() {
Collection<String> ret = getSorted(userAttrs.keySet());
- if (ret.contains(SCRIPT_FIELD__NAME)) {
+ if (ret.contains(SCRIPT_FIELD__NAME)) { // this is needed to avoid calling remove() on unmodifiable collection
ret.remove(SCRIPT_FIELD__NAME);
}
Review Comment:
The comment indicates this is to avoid calling remove() on an unmodifiable collection, but the code still calls remove(). If getSorted() returns an unmodifiable collection, this will throw an UnsupportedOperationException.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@ranger.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RANGER-5324: replace iterations with streams in RangerRequestScriptEvaluator [ranger]
Posted by "mneethiraj (via GitHub)" <gi...@apache.org>.
mneethiraj commented on code in PR #671:
URL: https://github.com/apache/ranger/pull/671#discussion_r2357011434
##########
agents-common/src/main/java/org/apache/ranger/plugin/policyengine/RangerRequestScriptEvaluator.java:
##########
@@ -443,125 +432,59 @@ public String getCurrentTagType() {
}
public Set<String> getAllTagTypes() {
- Set<String> allTagTypes = null;
- Set<RangerTagForEval> tagObjectList = getAllTags();
-
- if (CollectionUtils.isNotEmpty(tagObjectList)) {
- for (RangerTagForEval tag : tagObjectList) {
- String tagType = tag.getType();
- if (allTagTypes == null) {
- allTagTypes = new HashSet<>();
- }
- allTagTypes.add(tagType);
- }
- }
+ Set<RangerTagForEval> tags = getAllTags();
- return allTagTypes;
+ return CollectionUtils.isEmpty(tags) ? null : tags.stream().map(RangerTagForEval::getType).collect(Collectors.toSet());
}
public Map<String, String> getTagAttributes(final String tagType) {
- Map<String, String> ret = null;
-
- if (StringUtils.isNotBlank(tagType)) {
- Set<RangerTagForEval> tagObjectList = getAllTags();
-
- // Assumption: There is exactly one tag with given tagType in the list of tags - may not be true ***TODO***
- // This will get attributes of the first tagType that matches
- if (CollectionUtils.isNotEmpty(tagObjectList)) {
- for (RangerTagForEval tag : tagObjectList) {
- if (tag.getType().equals(tagType)) {
- ret = tag.getAttributes();
- break;
- }
- }
- }
- }
+ Set<RangerTagForEval> tags = StringUtils.isBlank(tagType) ? null : getAllTags();
- return ret;
+ // Assumption: There is exactly one tag with given tagType in the list of tags - may not be true ***TODO**
+ // This will get attributes of the first tagType that matches
+ return CollectionUtils.isEmpty(tags) ? null :
+ tags.stream()
+ .filter(tag -> tagType.equals(tag.getType()))
+ .findFirst()
+ .map(RangerTagForEval::getAttributes)
+ .orElse(null);
}
public List<Map<String, String>> getTagAttributesForAllMatchingTags(final String tagType) {
- List<Map<String, String>> ret = null;
-
- if (StringUtils.isNotBlank(tagType)) {
- Set<RangerTagForEval> tagObjectList = getAllTags();
-
- // Assumption: There is exactly one tag with given tagType in the list of tags - may not be true ***TODO***
- // This will get attributes of the first tagType that matches
- if (CollectionUtils.isNotEmpty(tagObjectList)) {
- for (RangerTagForEval tag : tagObjectList) {
- if (tag.getType().equals(tagType)) {
- Map<String, String> tagAttributes = tag.getAttributes();
- if (tagAttributes != null) {
- if (ret == null) {
- ret = new ArrayList<>();
- }
- ret.add(tagAttributes);
- }
- break;
- }
- }
- }
- }
+ Set<RangerTagForEval> tags = StringUtils.isBlank(tagType) ? null : getAllTags();
- return ret;
+ return CollectionUtils.isEmpty(tags) ? null :
+ tags.stream()
+ .filter(tag -> tagType.equals(tag.getType()))
+ .map(RangerTagForEval::getAttributes)
Review Comment:
@vyommani - here are 2 methods to get value of tag attribute:
1. `getAttributeValue(tagType, attrName)`: returns value of the attribute from the first tag instance
2. `getAttributeValueForAllMatchingTags(tagType, attrName)`: returns value of an attribute from all instances of a given tagType
Currently `getAttributeValueForAllMatchingTags(tagType, attrName)` is not used, hence the fact that it returns attribute value from the first instance only went undetected. This update fixes that issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@ranger.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RANGER-5324: replace iterations with streams in RangerRequestScriptEvaluator [ranger]
Posted by "vyommani (via GitHub)" <gi...@apache.org>.
vyommani commented on PR #671:
URL: https://github.com/apache/ranger/pull/671#issuecomment-3303221109
> I am not very familiar with streams but based on my past experience, streams can be much slower than a for loop / iterator and also creates GC pressure due to its own intermediate objects it creates. Exceptions to this is when using parallel streams where benefits of parallelism exist. We should evaluate performance impact of replacing loops with streams since auth/policy evaluation is very performance sensitive
Thanks for raising the performance considerations for auth/policy evaluation and concerns about streams, such as GC pressure from intermediate objects. However, streams can sometimes outperform traditional loops due to JVM optimizations like lazy evaluation and operation fusion, particularly for larger datasets. The performance impact depends on the specific workload, It will be great if you spent some time on this as Madhan suggested.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@ranger.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RANGER-5324: replace iterations with streams in RangerRequestScriptEvaluator [ranger]
Posted by "mneethiraj (via GitHub)" <gi...@apache.org>.
mneethiraj commented on code in PR #671:
URL: https://github.com/apache/ranger/pull/671#discussion_r2353571635
##########
agents-common/src/main/java/org/apache/ranger/plugin/policyengine/RangerRequestScriptEvaluator.java:
##########
@@ -365,7 +354,7 @@ public String getResourceZone() {
public Set<String> getResourceZones() {
Set<String> ret = RangerAccessRequestUtil.getResourceZoneNamesFromContext(getRequestContext());
- return ret != null ? Collections.emptySet() : ret;
+ return ret != null ? ret : Collections.emptySet();
Review Comment:
Yes. This fix is intentional.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@ranger.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RANGER-5324: replace iterations with streams in RangerRequestScriptEvaluator [ranger]
Posted by "fateh288 (via GitHub)" <gi...@apache.org>.
fateh288 commented on PR #671:
URL: https://github.com/apache/ranger/pull/671#issuecomment-3300550068
I am not very familiar with streams but based on my past experience, streams can be much slower than a for loop / iterator and also creates GC pressure due to its own intermediate objects it creates. Exceptions to this is when using parallel streams where benefits of parallelism exist.
We should evaluate performance impact of replacing loops with streams since auth/policy evaluation is very performance sensitive
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@ranger.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RANGER-5324: replace iterations with streams in RangerRequestScriptEvaluator [ranger]
Posted by "mneethiraj (via GitHub)" <gi...@apache.org>.
mneethiraj commented on PR #671:
URL: https://github.com/apache/ranger/pull/671#issuecomment-3300681927
> ... based on my past experience, streams can be much slower than a for loop / iterator and also creates GC pressure due to its own intermediate objects it creates. ...
@fateh288 - incorrect use of any feature can lead to poor performance :-) . That said, the changes in this PR don't look to introduce any performance issues. Can you please review and flag any potetial issue?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@ranger.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RANGER-5324: replace iterations with streams in RangerRequestScriptEvaluator [ranger]
Posted by "mneethiraj (via GitHub)" <gi...@apache.org>.
mneethiraj merged PR #671:
URL: https://github.com/apache/ranger/pull/671
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@ranger.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RANGER-5324: replace iterations with streams in RangerRequestScriptEvaluator [ranger]
Posted by "vyommani (via GitHub)" <gi...@apache.org>.
vyommani commented on code in PR #671:
URL: https://github.com/apache/ranger/pull/671#discussion_r2354759741
##########
agents-common/src/main/java/org/apache/ranger/plugin/policyengine/RangerRequestScriptEvaluator.java:
##########
@@ -443,125 +432,59 @@ public String getCurrentTagType() {
}
public Set<String> getAllTagTypes() {
- Set<String> allTagTypes = null;
- Set<RangerTagForEval> tagObjectList = getAllTags();
-
- if (CollectionUtils.isNotEmpty(tagObjectList)) {
- for (RangerTagForEval tag : tagObjectList) {
- String tagType = tag.getType();
- if (allTagTypes == null) {
- allTagTypes = new HashSet<>();
- }
- allTagTypes.add(tagType);
- }
- }
+ Set<RangerTagForEval> tags = getAllTags();
- return allTagTypes;
+ return CollectionUtils.isEmpty(tags) ? null : tags.stream().map(RangerTagForEval::getType).collect(Collectors.toSet());
}
public Map<String, String> getTagAttributes(final String tagType) {
- Map<String, String> ret = null;
-
- if (StringUtils.isNotBlank(tagType)) {
- Set<RangerTagForEval> tagObjectList = getAllTags();
-
- // Assumption: There is exactly one tag with given tagType in the list of tags - may not be true ***TODO***
- // This will get attributes of the first tagType that matches
- if (CollectionUtils.isNotEmpty(tagObjectList)) {
- for (RangerTagForEval tag : tagObjectList) {
- if (tag.getType().equals(tagType)) {
- ret = tag.getAttributes();
- break;
- }
- }
- }
- }
+ Set<RangerTagForEval> tags = StringUtils.isBlank(tagType) ? null : getAllTags();
- return ret;
+ // Assumption: There is exactly one tag with given tagType in the list of tags - may not be true ***TODO**
+ // This will get attributes of the first tagType that matches
+ return CollectionUtils.isEmpty(tags) ? null :
+ tags.stream()
+ .filter(tag -> tagType.equals(tag.getType()))
+ .findFirst()
+ .map(RangerTagForEval::getAttributes)
+ .orElse(null);
}
public List<Map<String, String>> getTagAttributesForAllMatchingTags(final String tagType) {
- List<Map<String, String>> ret = null;
-
- if (StringUtils.isNotBlank(tagType)) {
- Set<RangerTagForEval> tagObjectList = getAllTags();
-
- // Assumption: There is exactly one tag with given tagType in the list of tags - may not be true ***TODO***
- // This will get attributes of the first tagType that matches
- if (CollectionUtils.isNotEmpty(tagObjectList)) {
- for (RangerTagForEval tag : tagObjectList) {
- if (tag.getType().equals(tagType)) {
- Map<String, String> tagAttributes = tag.getAttributes();
- if (tagAttributes != null) {
- if (ret == null) {
- ret = new ArrayList<>();
- }
- ret.add(tagAttributes);
- }
- break;
- }
- }
- }
- }
+ Set<RangerTagForEval> tags = StringUtils.isBlank(tagType) ? null : getAllTags();
- return ret;
+ return CollectionUtils.isEmpty(tags) ? null :
+ tags.stream()
+ .filter(tag -> tagType.equals(tag.getType()))
+ .map(RangerTagForEval::getAttributes)
Review Comment:
don't you think we have to call findFirst() ? as you did in previous changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@ranger.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RANGER-5324: replace iterations with streams in RangerRequestScriptEvaluator [ranger]
Posted by "mneethiraj (via GitHub)" <gi...@apache.org>.
mneethiraj commented on code in PR #671:
URL: https://github.com/apache/ranger/pull/671#discussion_r2353683026
##########
agents-common/src/main/java/org/apache/ranger/plugin/policyengine/RangerRequestScriptEvaluator.java:
##########
@@ -1160,124 +1030,75 @@ private Map<String, String> copyMap(Map<String, String> obj) {
}
private List<Object> getUgAttr(String attrName) {
- List<Object> ret = new ArrayList<>();
-
- for (String groupName : userGroups) {
- Map<String, String> attrs = groupAttrs.get(groupName);
- Object val = attrs != null ? attrs.get(attrName) : null;
-
- if (val != null) {
- ret.add(val);
- }
- }
-
- return ret;
+ return userGroups.stream()
+ .map(groupAttrs::get)
+ .filter(Objects::nonNull)
+ .map(attrs -> attrs.get(attrName))
+ .filter(Objects::nonNull)
+ .collect(Collectors.toList());
}
private List<Object> getTagAttr(String attrName) {
- List<Object> ret = new ArrayList<>();
-
- for (String tagName : tagNames) {
- Map<String, Object> attrs = tags.get(tagName);
- Object val = attrs != null ? attrs.get(attrName) : null;
-
- if (val != null) {
- ret.add(val);
- }
- }
-
- return ret;
+ return tagNames.stream()
+ .map(tags::get)
+ .filter(Objects::nonNull)
+ .map(attrs -> attrs.get(attrName))
+ .filter(Objects::nonNull)
+ .collect(Collectors.toList());
}
private Collection<String> getUserAttrNames() {
Collection<String> ret = getSorted(userAttrs.keySet());
- if (ret.contains(SCRIPT_FIELD__NAME)) {
+ if (ret.contains(SCRIPT_FIELD__NAME)) { // this is needed to avoid calling remove() on unmodifiable collection
ret.remove(SCRIPT_FIELD__NAME);
}
Review Comment:
Good point. Addressed this in a subsequent commit in this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@ranger.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org