yihua commented on PR #19717: URL: https://github.com/apache/hudi/pull/19717#issuecomment-5687247616
Summary of the commits added since the approval on Aug 31, since the description still describes only the original change and the blast radius has grown past it. **Correctness fix in the resolver.** `resolvePartitionsToInit` reached its single-candidate branch with a request in hand only when the requested partition had no definition, and `uninitializedPartitions` only ever holds partitions that do, so that branch could only ever return a different index. A `HoodieIndexer` run naming `idx_b` on a table with an uninitialized `idx_a` built `idx_a` and then marked `idx_b` complete with nothing in it. A single candidate now answers a request only when it is the requested partition, which is the shape a first-time index takes once the lookup has minted its definition. Covered by `testFailsWhenTheOnlyUninitializedPartitionIsNotTheRequestedOne` and `testInitializesTheRequestedPartitionWhoseDefinitionTheLookupMinted`; verified the first fails without the guard. **`BaseHoodieTableServiceClient.getInstantsToRollback` now exempts solo-family instants while an indexing action is pending.** This is the one worth a close look, because it is shared write-path code rather than indexing code, and the description does not mention that this PR touches it at all. Eager rollback spared an inflight metadata deltacommit only when it was the indexing action's own instant, so moving the bootstrap onto a solo instant dropped that protection: a concurrent ingest writer's metadata client is EAGER by default and its `startCommit` reaches that filter, so it can roll a long-running bootstrap back mid-flight. The exemption is conditioned on a pending indexing action so solo instants stay rollback-eligible otherwise. I could not find a cheap way to test it, since the EAGER metadata branch needs a real data table at the derived dataset base path and the existing `getInstantsToRollback_*` tests only cover the LAZY clustering branch, so this one is reasoned rather than executed. **Two smaller ones.** The definition lookup was the resolver argument, so it ran before the already-initialized skip could decide to do nothing and could persist a definition on a run that built nothing, which is itself the precondition for the wrong-index case above; it is now a supplier evaluated only when inference is needed. And the resolver's throw is a `HoodieMetadataException`, which `RunIndexActionExecutor` did not catch, so `abort()` never ran and the requested partition stranded inflight in the table config with the index instant inflight, which neither a re-run nor a re-schedule can get past; it now aborts like an IO failure and re-reads the table config before adding to it. One scope question worth deciding before merge: `HoodieIndexer.doSchedule` enforces `partitionTypes.size() == 1` and the scheduler derives one partition name per type, so an indexing action initializes exactly one definition-driven partition, which means the same-instant collision the solo-family change guards against is not reachable through the shipped entry points today. That change is the one that created the eager-rollback hazard above, so it may be worth keeping it deliberately as hardening or dropping it, rather than carrying both it and its mitigation. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
