HDDS-8913. ContainerManagerImpl: reduce processing while locked

[adoroszlai]

What changes were proposed in this pull request?

1. ContainerManagerImpl lock is intended only for write operations (see comment 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

[sodonnel]

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

[adoroszlai]

Thanks for the suggestion, I'll update the patch with it.

FYI there is a bug in that solution due to PriorityQueue#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.

[sodonnel]

I wonder how many people have not noticed that issue, and just implemented the linked solution as is!

[sodonnel]

LGTM - Nice work making the topN generic for future usage!

[adoroszlai]

Thanks @siddhantsangwan, @sodonnel for the review.