You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "adoroszlai (via GitHub)" <gi...@apache.org> on 2023/06/23 14:38:40 UTC
[GitHub] [ozone] adoroszlai opened a new pull request, #4967: HDDS-8913. ContainerManagerImpl: reduce processing while locked
adoroszlai opened a new pull request, #4967:
URL: https://github.com/apache/ozone/pull/4967
## What changes were proposed in this pull request?
1. `ContainerManagerImpl` lock is intended only for write operations (see [comment](https://github.com/apache/ozone/blob/3211547ed21b7683720ca57282e13a4edb9fdce2/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerManagerImpl.java#L68-L71) on `lock`). `ContainerStateManagerImpl` has its own read/write lock. Earlier `ConcurrentModificationException` (HDDS-6314) happened because `ContainerStateManagerImpl` returns an unmodifiable view of its internal sets. These are not thread-safe, only protect against changing the set. Replaced these with `ImmutableSet.copyOf`.
2. Replace stream with iteration.
3. Remove ineligible items from the list being sorted before sorting it.
4. Only filter items if request is not for all items (as indicated by the smallest possible `ContainerID`: 0.
5. Increment metric for `getContainers(LifeCycleState)`, was missing previously.
6. Move metrics calls out of locked block in `deleteContainer`.
Further improvement is possible, as some of the container sets are already sorted (to be done separately in HDDS-6315).
https://issues.apache.org/jira/browse/HDDS-8913
## How was this patch tested?
CI:
https://github.com/adoroszlai/hadoop-ozone/actions/runs/5356645847
--
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] siddhantsangwan commented on a diff in pull request #4967: HDDS-8913. ContainerManagerImpl: reduce processing while locked
Posted by "siddhantsangwan (via GitHub)" <gi...@apache.org>.
siddhantsangwan commented on code in PR #4967:
URL: https://github.com/apache/ozone/pull/4967#discussion_r1243306663
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerManagerImpl.java:
##########
@@ -144,55 +143,23 @@ public ContainerInfo getContainer(final ContainerID id)
public List<ContainerInfo> getContainers(final ContainerID startID,
final int count) {
scmContainerManagerMetrics.incNumListContainersOps();
- final List<ContainerID> containersIds =
- new ArrayList<>(containerStateManager.getContainerIDs());
- Collections.sort(containersIds);
- List<ContainerInfo> containers;
- lock.lock();
- try {
- containers = containersIds.stream()
- .filter(id -> id.compareTo(startID) >= 0).limit(count)
- .map(containerStateManager::getContainer)
- .collect(Collectors.toList());
- } finally {
- lock.unlock();
- }
- return containers;
+ return toContainers(filterSortAndLimit(startID, count,
Review Comment:
I noticed that this method's interface javadoc says the List should not include the `startID`.
```
/**
* Returns containers under certain conditions.
* Search container IDs from start ID(exclusive),
* The max size of the searching range cannot exceed the
* value of count.
```
The previous implementation also includes the `startID`, so perhaps we should edit the javadoc.
--
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 diff in pull request #4967: HDDS-8913. ContainerManagerImpl: reduce processing while locked
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #4967:
URL: https://github.com/apache/ozone/pull/4967#discussion_r1243648023
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerManagerImpl.java:
##########
@@ -471,4 +444,37 @@ public ContainerStateManager getContainerStateManager() {
public SCMHAManager getSCMHAManager() {
return haManager;
}
+
+ private static List<ContainerID> filterSortAndLimit(
Review Comment:
Thanks for the suggestion, I'll update the patch with it.
FYI there is a bug in that solution due to [`PriorityQueue#iterator()`](https://docs.oracle.com/javase/8/docs/api/java/util/PriorityQueue.html#iterator--):
> The iterator does not return the elements in any particular order.
so `new ArrayList<>(maxHeap)` randomizes the result.
It seemsto work for (small?) `Integers`, but not for any random `Comparable`.
--
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 diff in pull request #4967: HDDS-8913. ContainerManagerImpl: reduce processing while locked
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #4967:
URL: https://github.com/apache/ozone/pull/4967#discussion_r1243648023
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerManagerImpl.java:
##########
@@ -471,4 +444,37 @@ public ContainerStateManager getContainerStateManager() {
public SCMHAManager getSCMHAManager() {
return haManager;
}
+
+ private static List<ContainerID> filterSortAndLimit(
Review Comment:
Thanks for the suggestion, I'll update the patch with it.
FYI there is a bug in that solution due to [`PriorityQueue#iterator()`](https://docs.oracle.com/javase/8/docs/api/java/util/PriorityQueue.html#iterator--):
> The iterator does not return the elements in any particular order.
so `new ArrayList<>(maxHeap)` randomizes the result.
It seems to work for (small?) `Integer`s, but not for any random `Comparable`.
--
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] sodonnel commented on a diff in pull request #4967: HDDS-8913. ContainerManagerImpl: reduce processing while locked
Posted by "sodonnel (via GitHub)" <gi...@apache.org>.
sodonnel commented on code in PR #4967:
URL: https://github.com/apache/ozone/pull/4967#discussion_r1243550458
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerManagerImpl.java:
##########
@@ -471,4 +444,37 @@ public ContainerStateManager getContainerStateManager() {
public SCMHAManager getSCMHAManager() {
return haManager;
}
+
+ private static List<ContainerID> filterSortAndLimit(
Review Comment:
I think this could be made more efficient. The current algorithm takes the set, forms a new list of all elements. Then filters, sorted and returns the range.
Using an adaption of the priorityQueue technique here - https://www.baeldung.com/java-array-top-elements
We could avoid the copy from set to list, and the potentially large sort.
```
for (contaienrID cid : set) {
if (id > startID)
maxHeap.add(number);
if (maxHeap.size() > count) {
maxHeap.poll();
}
}
});
return new ArrayList<>(maxHeap);
// Might need reversed, I am not sure
```
I think this would be more memory efficient on large container sets, and should perform better than the full sort
--
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] siddhantsangwan commented on a diff in pull request #4967: HDDS-8913. ContainerManagerImpl: reduce processing while locked
Posted by "siddhantsangwan (via GitHub)" <gi...@apache.org>.
siddhantsangwan commented on code in PR #4967:
URL: https://github.com/apache/ozone/pull/4967#discussion_r1243631096
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerManagerImpl.java:
##########
@@ -471,4 +444,37 @@ public ContainerStateManager getContainerStateManager() {
public SCMHAManager getSCMHAManager() {
return haManager;
}
+
+ private static List<ContainerID> filterSortAndLimit(
Review Comment:
Do we even need to sort the list? The interface javadoc does not say that the returned list will be sorted. I know the earlier implementation is sorting, and there might be other code that's already relying on a sorted order... But I think it should be the caller's responsibility to manipulate the data however they want.
--
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 diff in pull request #4967: HDDS-8913. ContainerManagerImpl: reduce processing while locked
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #4967:
URL: https://github.com/apache/ozone/pull/4967#discussion_r1243327629
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerManagerImpl.java:
##########
@@ -144,55 +143,23 @@ public ContainerInfo getContainer(final ContainerID id)
public List<ContainerInfo> getContainers(final ContainerID startID,
final int count) {
scmContainerManagerMetrics.incNumListContainersOps();
- final List<ContainerID> containersIds =
- new ArrayList<>(containerStateManager.getContainerIDs());
- Collections.sort(containersIds);
- List<ContainerInfo> containers;
- lock.lock();
- try {
- containers = containersIds.stream()
- .filter(id -> id.compareTo(startID) >= 0).limit(count)
- .map(containerStateManager::getContainer)
- .collect(Collectors.toList());
- } finally {
- lock.unlock();
- }
- return containers;
+ return toContainers(filterSortAndLimit(startID, count,
Review Comment:
I'd rather leave that for another patch, as I'm not touching the interface or changing behavior here. The mismatch seems to have been introduced with SCM HA implementation.
--
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] sodonnel commented on a diff in pull request #4967: HDDS-8913. ContainerManagerImpl: reduce processing while locked
Posted by "sodonnel (via GitHub)" <gi...@apache.org>.
sodonnel commented on code in PR #4967:
URL: https://github.com/apache/ozone/pull/4967#discussion_r1243796973
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerManagerImpl.java:
##########
@@ -471,4 +444,37 @@ public ContainerStateManager getContainerStateManager() {
public SCMHAManager getSCMHAManager() {
return haManager;
}
+
+ private static List<ContainerID> filterSortAndLimit(
Review Comment:
I wonder how many people have not noticed that issue, and just implemented the linked solution as is!
--
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 #4967: HDDS-8913. ContainerManagerImpl: reduce processing while locked
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai merged PR #4967:
URL: https://github.com/apache/ozone/pull/4967
--
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] sodonnel commented on a diff in pull request #4967: HDDS-8913. ContainerManagerImpl: reduce processing while locked
Posted by "sodonnel (via GitHub)" <gi...@apache.org>.
sodonnel commented on code in PR #4967:
URL: https://github.com/apache/ozone/pull/4967#discussion_r1243550458
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerManagerImpl.java:
##########
@@ -471,4 +444,37 @@ public ContainerStateManager getContainerStateManager() {
public SCMHAManager getSCMHAManager() {
return haManager;
}
+
+ private static List<ContainerID> filterSortAndLimit(
Review Comment:
I think this could be made more efficient. The current algorithm takes the set, forms a new list of all elements. Then filters, sorted and returns the range.
Using an adaption of the priorityQueue technique here - https://www.baeldung.com/java-array-top-elements
We could avoid the copy from set to list, and the potentially large sort.
```
for (contaienrID cid : set) {
if (id > startID)
maxHeap.add(number);
if (maxHeap.size() > count) {
maxHeap.poll();
}
}
return new ArrayList<>(maxHeap);
// Might need reversed, I am not sure
});
```
I think this would be more memory efficient on large container sets, and should perform better than the full sort
--
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 diff in pull request #4967: HDDS-8913. ContainerManagerImpl: reduce processing while locked
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #4967:
URL: https://github.com/apache/ozone/pull/4967#discussion_r1243648023
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerManagerImpl.java:
##########
@@ -471,4 +444,37 @@ public ContainerStateManager getContainerStateManager() {
public SCMHAManager getSCMHAManager() {
return haManager;
}
+
+ private static List<ContainerID> filterSortAndLimit(
Review Comment:
Thanks for the suggestion, I'll update the patch with it.
FYI there is a bug in that solution due to [`PriorityQueue#iterator()`](https://docs.oracle.com/javase/8/docs/api/java/util/PriorityQueue.html#iterator--):
> The iterator does not return the elements in any particular order.
so `new ArrayList<>(maxHeap)` randomizes the result.
It seems to work for (small?) `Integers`, but not for any random `Comparable`.
--
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 #4967: HDDS-8913. ContainerManagerImpl: reduce processing while locked
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #4967:
URL: https://github.com/apache/ozone/pull/4967#issuecomment-1610004818
Thanks @siddhantsangwan, @sodonnel 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