You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2021/12/07 14:34:30 UTC
[GitHub] [ozone] adoroszlai opened a new pull request #2895: HDDS-6071. ResourceLimitCache leaks permits
adoroszlai opened a new pull request #2895:
URL: https://github.com/apache/ozone/pull/2895
## What changes were proposed in this pull request?
`ResourceLimitCache` limits its size by using a group of permits. `put` requires free permit before it can add data. However, `removeIf` does not release permits.
https://issues.apache.org/jira/browse/HDDS-6071
## How was this patch tested?
Added unit test.
Also, previously `TestRandomKeyGenerator` has been [timing out](https://github.com/apache/ozone/runs/4440507728?check_suite_focus=true#step:4:3010) since HDDS-3227 was merged. It is passing with the patch.
CI:
https://github.com/adoroszlai/hadoop-ozone/actions/runs/1549379969
https://github.com/adoroszlai/hadoop-ozone/actions/runs/1549427103
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on a change in pull request #2895: HDDS-6071. ResourceLimitCache leaks permits
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on a change in pull request #2895:
URL: https://github.com/apache/ozone/pull/2895#discussion_r764565958
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/utils/ResourceLimitCache.java
##########
@@ -79,7 +79,11 @@ public V remove(K key) {
@Override
public void removeIf(Predicate<K> predicate) {
Objects.requireNonNull(predicate);
- map.keySet().removeIf(predicate);
+ for (K key : new ArrayList<>(map.keySet())) {
Review comment:
Thanks @lokeshj1703 for the review.
We could loop over `map.keySet()` directly, the `for-each` loop (`:`) uses iterator behind the scenes either way. I wrapped key set in the list to avoid concurrent modification issues.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] lokeshj1703 commented on a change in pull request #2895: HDDS-6071. ResourceLimitCache leaks permits
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on a change in pull request #2895:
URL: https://github.com/apache/ozone/pull/2895#discussion_r764556338
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/utils/ResourceLimitCache.java
##########
@@ -79,7 +79,11 @@ public V remove(K key) {
@Override
public void removeIf(Predicate<K> predicate) {
Objects.requireNonNull(predicate);
- map.keySet().removeIf(predicate);
+ for (K key : new ArrayList<>(map.keySet())) {
Review comment:
We can use iterator here to replace ArrayList.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] lokeshj1703 commented on a change in pull request #2895: HDDS-6071. ResourceLimitCache leaks permits
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on a change in pull request #2895:
URL: https://github.com/apache/ozone/pull/2895#discussion_r764597608
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/utils/ResourceLimitCache.java
##########
@@ -79,7 +79,11 @@ public V remove(K key) {
@Override
public void removeIf(Predicate<K> predicate) {
Objects.requireNonNull(predicate);
- map.keySet().removeIf(predicate);
+ for (K key : new ArrayList<>(map.keySet())) {
Review comment:
I agree. Maybe we should replace the for loop with `ConcurrentMap.forEach(BiConsumer<? super K, ? super V> action)`. Since the method is exposed by the interface it should provide the thread safety guarantees.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on a change in pull request #2895: HDDS-6071. ResourceLimitCache leaks permits
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on a change in pull request #2895:
URL: https://github.com/apache/ozone/pull/2895#discussion_r764596287
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/utils/ResourceLimitCache.java
##########
@@ -79,7 +79,11 @@ public V remove(K key) {
@Override
public void removeIf(Predicate<K> predicate) {
Objects.requireNonNull(predicate);
- map.keySet().removeIf(predicate);
+ for (K key : new ArrayList<>(map.keySet())) {
Review comment:
Yes, `ConcurrentHashMap` doc says that its iterator does not throw `ConcurrentModificationException`. However, `ResourceLimitCache` accepts any `ConcurrentMap`, and this interface does not state the same (i.e. I am not sure it is a requirement for all implementations). So I preferred to be on the safe side.
Since `clear()` already iterates `keySet` directly, I guess we can still get rid of the wrapping `ArrayList`, so I have updated the patch.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] smengcl commented on a change in pull request #2895: HDDS-6071. ResourceLimitCache leaks permits
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #2895:
URL: https://github.com/apache/ozone/pull/2895#discussion_r764600159
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/utils/ResourceLimitCache.java
##########
@@ -79,7 +79,11 @@ public V remove(K key) {
@Override
public void removeIf(Predicate<K> predicate) {
Objects.requireNonNull(predicate);
- map.keySet().removeIf(predicate);
+ for (K key : new ArrayList<>(map.keySet())) {
Review comment:
`ConcurrentMap#forEach` doesn't throw `ConcurrentModificationException` like `Map#forEach` does. I think it might be fine.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] smengcl commented on a change in pull request #2895: HDDS-6071. ResourceLimitCache leaks permits
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #2895:
URL: https://github.com/apache/ozone/pull/2895#discussion_r764597015
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/utils/ResourceLimitCache.java
##########
@@ -79,7 +79,11 @@ public V remove(K key) {
@Override
public void removeIf(Predicate<K> predicate) {
Objects.requireNonNull(predicate);
- map.keySet().removeIf(predicate);
+ for (K key : new ArrayList<>(map.keySet())) {
Review comment:
It might be safe to just skip the array copy here? As we already did in `clear()` below.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on pull request #2895: HDDS-6071. ResourceLimitCache leaks permits
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on pull request #2895:
URL: https://github.com/apache/ozone/pull/2895#issuecomment-988701641
Thanks @bshashikant, @lokeshj1703 and @smengcl for the review.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai merged pull request #2895: HDDS-6071. ResourceLimitCache leaks permits
Posted by GitBox <gi...@apache.org>.
adoroszlai merged pull request #2895:
URL: https://github.com/apache/ozone/pull/2895
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] lokeshj1703 commented on a change in pull request #2895: HDDS-6071. ResourceLimitCache leaks permits
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on a change in pull request #2895:
URL: https://github.com/apache/ozone/pull/2895#discussion_r764592658
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/utils/ResourceLimitCache.java
##########
@@ -79,7 +79,11 @@ public V remove(K key) {
@Override
public void removeIf(Predicate<K> predicate) {
Objects.requireNonNull(predicate);
- map.keySet().removeIf(predicate);
+ for (K key : new ArrayList<>(map.keySet())) {
Review comment:
I think we can use a combination of forEach and remove mehods exposed by ConcurrentMap interface. Since ConcurrentMap operations are thread safe, it should not throw concurrent modification errors.
Ref: https://stackoverflow.com/questions/3768554/is-iterating-concurrenthashmap-values-thread-safe
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org