Uh oh!
There was an error while loading. Please reload this page.
HDDS-12849. On demand container scanner should not be static - #8296
Conversation
aswinshakil
left a comment
There was a problem hiding this comment.
Thanks @errose28 for working on this. Overall, the changes look good. I have left some minor comments.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| } | ||
| OnDemandContainerDataScanner.init(c, controller); | ||
| onDemandScanner = new OnDemandContainerDataScanner(c, controller); | ||
| containerSet.registerContainerScanHandler(onDemandScanner::scanContainer); |
There was a problem hiding this comment.
Can we pass the OnDemandContainerDataScanner itself here instead of using Consumer?
There was a problem hiding this comment.
We could, but I would rather keep the coupling between the two classes looser. We could make a scanner interface but it would only have one method and only be used here. The consumer was a light-weight way to get this same functionality.
errose28
commented
Apr 21, 2025
Thanks for the review @aswinshakil. Comments have been addressed in the latest commit. |
aswinshakil
left a comment
There was a problem hiding this comment.
Thanks @errose28 for the update. LGTM. Pending CI
…anner-builds-mt * HDDS-10239-container-reconciliation: HDDS-12849. On demand container scanner should not be static (apache#8296) HDDS-12745. Container checksum should be reported during container close and DN restart (apache#8204) Conflicts: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/OnDemandContainerDataScanner.java hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/OzoneContainer.java hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/impl/TestContainerSet.java hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/ozoneimpl/TestOnDemandContainerDataScanner.java
What changes were proposed in this pull request?
The on demand container scanner was originally implemented as a static instance so that it could be easily triggered from anywhere in the code. This is now causing problems for reconciliation testing where multiple replicas exist in the same process. We are invoking on demand scans for each replica after it is reconciled, and these scans for different replicas are hitting the same container scanner concurrently. This is required for tests to pass in #7490.
Due to the layout of the datanode code, it is difficult to get items like this to all places where they are needed, which is why the on demand scanner was initially static. I have attempted to remedy this issue by adding a scan handler which can be triggered from the
ContainerSet.ContainerSetwas chosen because it is both created by theOzoneContainerwhich also initializes the scanners, and available in most locations in the datanode.What is the link to the Apache JIRA
HDDS-12849
How was this patch tested?
Existing unit and integration tests for on demand container scanner pass. New unit test added.