fix: harden async indexing and improve coverage - #19537
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #19537 +/- ##
============================================
+ Coverage 76.40% 76.78% +0.37%
+ Complexity 32398 32316 -82
============================================
Files 2520 2520
Lines 138985 139058 +73
Branches 16695 16748 +53
============================================
+ Hits 106189 106771 +582
+ Misses 25166 24675 -491
+ Partials 7630 7612 -18
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for working on this! This PR is test-only, adding focused unit and fixture-backed coverage for legacy upgrade/downgrade handlers, the upgrade orchestration flow, async-index scheduling/execution/catch-up, and the simple/consistent bucket-index utilities in hudi-client-common. I traced each new test through the production code it exercises (mock chains, assertion values, and exception/abort paths) and the tests genuinely drive the intended paths without passing vacuously or asserting wrong behavior. No correctness issues found. A few style/readability suggestions in the inline comments. Please take a look, and this should be ready for a Hudi committer or PMC member to take it from here. One small static-import inconsistency in TestLegacyUpgradeDowngradeHandlers; everything else looks clean.
cc @yihua
| when(config.getMarkersDeleteParallelism()).thenReturn(3); | ||
|
|
||
| ZeroToOneUpgradeHandler handler = org.mockito.Mockito.spy(new ZeroToOneUpgradeHandler()); | ||
| org.mockito.Mockito.doNothing().when(handler).recreateMarkers(anyString(), eq(table), eq(context), anyInt()); |
There was a problem hiding this comment.
🤖 nit: org.mockito.Mockito.spy and org.mockito.Mockito.doNothing() are used fully-qualified on these two lines while the file already statically imports mock, verify, doReturn, etc. — could you add spy and doNothing to the static imports for consistency?
There was a problem hiding this comment.
Addressed in d788769: added static imports for spy and doNothing and removed the fully qualified Mockito calls. The updated module passes Checkstyle.
cshuo
left a comment
cshuo
left a comment
There was a problem hiding this comment.
I found two gaps in the new async-index coverage: the catch-up tests mask stale progress propagation, and the FILES initialization path leaves its metadata writer unclosed. Details are inline.
| @Override | ||
| public void updateIndexForWriteAction(HoodieInstant instant) { | ||
| writeActionsUpdated.incrementAndGet(); | ||
| currentCaughtupInstant = instant.requestedTime(); |
There was a problem hiding this comment.
P1: This test double advances currentCaughtupInstant itself, but neither production catch-up implementation does so. The executor test also supplies empty timelines and mocks IndexingCatchupTask, so it proves only that run() was called. In production, RunIndexActionExecutor passes the initial instant as a String and later builds HoodieIndexCommitMetadata from its unchanged field; after processing completed instant 002, it still reports 001. Please exercise a production catch-up task with a real completed instant, assert the returned indexUptoInstant, and propagate the last successfully processed instant back to the executor.
There was a problem hiding this comment.
Addressed in d788769. AbstractIndexingCatchupTask now advances progress after every successfully processed completed action, and RunIndexActionExecutor reads that progress after Future.get() before building HoodieIndexCommitMetadata. The executor regression now runs the production write-stat catch-up task against a real completed 002 commit and asserts indexUptoInstant is 002. The full PR-focused suite passes all 46 tests.
| 1, MetadataPartitionType.FILES.getPartitionPath(), "001", Collections.emptyMap()); | ||
| when(activeTimeline.readIndexPlan(requested)).thenReturn(new HoodieIndexPlan(1, Collections.singletonList(info))); | ||
| HoodieTableMetadataWriter writer = mock(HoodieTableMetadataWriter.class); | ||
| when(table.getIndexingMetadataWriter("002")).thenReturn(Option.of(writer)); |
There was a problem hiding this comment.
P2: This mock represents an AutoCloseable metadata writer, but the FILES initialization branch obtains the writer and discards it without closing it. Unlike the non-FILES branch, there is no try-with-resources; the real writer's close() releases its metadata reader, write client, and view. A focused verify(writer).close() assertion fails. Please close the writer in the production FILES branch and assert closure on both successful initialization and timeline-completion failure.
There was a problem hiding this comment.
Addressed in d788769. The FILES initialization branch now owns the metadata writer with try-with-resources, and the tests verify close() after both successful initialization and a saveAsComplete timeline failure. The full PR-focused suite passes all 46 tests.
danny0405
changed the title
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for working on this! The PR propagates the last caught-up instant from the async-index catch-up task back to RunIndexActionExecutor (so index commit metadata no longer retains the stale base instant), closes the metadata writer used during FILES-partition initialization, and adds substantial coverage for legacy upgrade/downgrade handlers, upgrade orchestration, async-index execution/catch-up, and bucket-index utilities. I traced the propagation path (the final indexUptoInstant cannot regress below the base instant; advancing past no-heartbeat instants is semantically sound; the value is safely published via Future.get()) and confirmed both concrete catch-up subclasses inherit the new interface method without overriding run(). No correctness issues found. A few style/readability suggestions in the inline comments. Please take a look, and this should be ready for a Hudi committer or PMC member to take it from here.
. One minor readability suggestion on the empty try-with-resources block; the rest of the changes (mostly new test coverage) look clean.
cc @yihua
| // save index commit metadata and update table config | ||
| // instantiation of metadata writer will automatically instantiate the partitions. | ||
| table.getIndexingMetadataWriter(instantTime) | ||
| try (HoodieTableMetadataWriter metadataWriter = table.getIndexingMetadataWriter(instantTime) |
There was a problem hiding this comment.
🤖 nit: an empty try-with-resources body that relies on the writer's construction side-effect reads as unusual. Could you make the intent more obvious, e.g. assign to a clearly-named local (initializingWriter) or keep the explanatory comment but consider whether an explicit .get()/no-op would read better?
5 participants
Describe the issue this Pull Request addresses
Several table upgrade/downgrade handlers, async-index actions, and bucket-index utilities in
hudi-client-commonhad significant line-coverage gaps. Critical paths such as legacy table-version transitions, index catch-up with concurrent commits, abort cleanup, bucket assignment, and consistent-hashing metadata recovery were not exercised directly.The added async-index coverage also exposed two correctness issues: catch-up progress was not propagated back to the executor, so index commit metadata could retain the original base instant after later commits were processed; and the metadata writer used while initializing the FILES partition was not closed.
Summary and Changelog
RunIndexActionExecutorand record it inHoodieIndexCommitMetadata.indexUptoInstant.ZeroToOneUpgradeHandler,TwoToOneDowngradeHandler,FourToFiveUpgradeHandler, andFiveToSixUpgradeHandler, including config and file-layout transitions and failure paths.TestUpgradeDowngradewith fixture-backed v4-to-v5 and v5-to-v6 upgrades and verify data consistency after each transition.UpgradeDowngradeversion-hop orchestration, rollback/compaction selection, property changes, no-op validation, and failure handling.Per-class line coverage:
ZeroToOneUpgradeHandlerTwoToOneDowngradeHandlerFourToFiveUpgradeHandlerFiveToSixUpgradeHandlerUpgradeDowngradeAbstractIndexingCatchupTaskRunIndexActionExecutorScheduleIndexActionExecutorHoodieSimpleBucketIndexConsistentBucketIndexUtils* The focused report excludes pre-existing suites; merged Codecov coverage retains their existing contribution.
Validation:
mvn -pl hudi-client/hudi-client-common -Punit-tests -Drat.skip=true -Dtest=TestLegacyUpgradeDowngradeHandlers,TestIndexingCatchupTask,TestIndexActionExecutors,TestHoodieSimpleBucketIndex,TestConsistentBucketIndexUtils,TestUpgradeDowngradeOrchestration test— 46 tests passed.mvn -pl hudi-spark-datasource/hudi-spark -Dcheckstyle.skip -Drat.skip=true -Djacoco.skip=true -Dtest=TestUpgradeDowngrade#testLegacyUpgradeHandlersWithFixtureTables surefire:test— 2 tests passed.mvn -pl hudi-client/hudi-client-common checkstyle:check— 0 violations.mvn -pl hudi-spark-datasource/hudi-spark checkstyle:check— 0 violations.git diff --check— passed.Impact
Async index commits now report the latest successfully caught-up data instant, and FILES metadata initialization releases its writer resources deterministically. The catch-up task exposes that internal progress to the executor. There are no configuration, storage-format, user-facing API, or performance changes.
Risk Level
low
The production changes are localized to async-index completion and resource cleanup. The focused suite exercises completed-commit catch-up, writer closure on success and failure, abort cleanup, and index commit metadata generation.
Documentation Update
none — there are no new features, configurations, storage formats, or user-facing behavior to document.
Contributor's checklist