Repository navigation
Conversation
chihsuan
left a comment
chihsuan
left a comment
There was a problem hiding this comment.
Thanks for working on this! @rich7420 the improvement looks promising. 👍
Would it make sense to add a multi-worker test with a low flush threshold to verify that repeated map replacements do not lose or duplicate data?
Optionally, I think it would be useful to benchmark a high number of buckets to measure peak memory usage, since each worker caches every bucket it encounters until reprocessing finishes.
| getReconNamespaceSummaryManager().clearNSSummaryTable(); | ||
|
|
||
| assertThat(task.reprocessWithOBS(omMetadataManager)).isTrue(); | ||
| verify(bucketTable, times(2)).getSkipCache(anyString()); |
There was a problem hiding this comment.
Would it be possible to make the bucket/key setup local to this test? The hard-coded times(2) currently depends on the shared fixture containing exactly two buckets, so an unrelated fixture change could break this test.
There was a problem hiding this comment.
Now captures the getSkipCache args and asserts doesNotHaveDuplicates() (each bucket fetched at most once), so it no longer depends on the bucket count. Also moved into a @Nested class.
…allback cleanup - Add testReprocessLowThresholdConservesData: with a flush threshold of 1, reprocess still yields the exact per-bucket file counts and sizes, so the repeated per-worker map replacement, async flush and end-of-loop drain neither lose nor double-count keys. - Replace the times(2) bucket-cache assertion, which depended on the fixture holding exactly two buckets, with an ArgumentCaptor + doesNotHaveDuplicates check that each bucket is looked up at most once, independent of bucket count. - Group both in a @nested class so they are discovered and reported clearly under a class-level run. - Document why the per-worker ThreadLocals are cleared in a finally: the small-table fallback in ParallelTableIteratorOperation runs the per-key operation on the reused Recon pool thread, so the finally is not dead code. Claude-Session: https://claude.ai/code/session_01FUpCUnmy6JzHPGvwMhyGzq
|
Thanks for the patch. I would like to defer this after #11754, which cuts the |
|
@adoroszlai ok problem. thanks! |
chihsuan
left a comment
chihsuan
left a comment
There was a problem hiding this comment.
Thanks for addressing my earlier comments! @rich7420 The new tests look good.
Sorry, I should have checked this last time. I tested each change on its own. The bucket cache makes reprocess faster, from about 2100ms to 1800ms. The ThreadLocal change doesn't make it faster, and it adds a second map we need to keep in sync.
Since only the bucket cache makes it faster, could we remove the ThreadLocal change from this PR?
What changes were proposed in this pull request?
OBS namespace summary reprocess caches the last bucket's metadata per worker and reuses its object ID, avoiding repeated bucket-table reads for consecutive keys. The cache checks both volume and bucket names and retains at most one bucket per worker.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-16281
How was this patch tested?
With JDK 21:
mvn -pl :ozone-recon -am clean install -DskipTests -DskipShade -DskipRecon -DskipDocs.mvn -pl :ozone-recon test '-Dtest=TestNSSummaryTaskWithOBS*,TestNSSummaryTaskWithFSO*,TestNSSummaryTaskWithLegacy*,TestContainerKeyMapperTask,TestFileSizeCountTask' -DskipShade -DskipRecon -DskipDocs -Ddevelocity.cache.local.enabled=false -Ddevelocity.cache.remote.enabled=false: 82 tests run, 0 failures, 0 errors, 0 skipped../hadoop-ozone/dev-support/checks/checkstyle.sh: 0 violations.CI: