nsivabalan commented on code in PR #19802:
URL: https://github.com/apache/hudi/pull/19802#discussion_r4226254148
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/metadata/HoodieBackedTableMetadataWriter.java:
##########
@@ -523,16 +523,14 @@ String generateUniqueInstantTime(String
initializationTime) {
protected abstract EngineType getEngineType();
- private Lazy<List<FileSliceAndPartition>> getLazyMergedFileSlices() {
+ private Lazy<List<FileSliceAndPartition>> getLazyMergedFileSlices(String
maxInstantTime) {
Review Comment:
I walked the code independently and agree with all of this. Rather than
restate it, here is what I was able to add: the interleaving reproduces, and it
is worse than the comment suggests for column stats.
**Both gaps reproduce deterministically on master.** I put the suite up as a
WIP draft in #20246 — it is intentionally red, no production change, two of the
seven tests fail on master and those two are the bugs. It that holds a writer
and the indexer in a pinned interleaving (`CountDownLatch` pairs rather than
hoping for a race), and asserts on index content rather than on the presence of
a metadata deltacommit — since a deltacommit written before the new partition
existed is indistinguishable from one that wrote it, which is the assumption
under test.
A commit whose write client reads the table config before the indexer sets
the inflight marker, and which commits during the run:
```
testCommitOverlappingRunStartIsIndexed
keys from a commit overlapping the indexer run are missing
from the record index: 50 of 50 keys
```
Every key from that commit, gone from the RLI. 4/4 runs, same assertion.
**Column stats fail the same way, not the milder way.** The comment reads as
though col stats and bloom filters lose a commit between schedule and execute
while RLI loses all of a commit's keys. In the straddling case both lose
everything, through the identical mechanism:
```
testFilesOnlyDeltacommitIsNotTreatedAsIndexed
column stats are missing for files written by a commit whose metadata
deltacommit
predates the new partition; catch-up accepted deltacommit existence as
proof of indexing:
[(2016/03/15, 9aa65ec1-...parquet),
(2015/03/16, 98e14227-...parquet),
(2015/03/17, 32f76d7c-...parquet)]
```
All three base files the commit wrote, zero column stats entries. 4/4 runs.
Worth noting because it widens the data-skipping exposure beyond what the RLI
case already implies.
**Two things I was able to rule out**, which I think usefully narrows where
the problem lives:
*Index scheduling vs. table-service scheduling is safe.* I checked whether a
compaction or clean plan being built in memory could race index planning. Both
take the same `txnManager` state-change lock around plan-and-persist
(`BaseHoodieTableServiceClient#739` and `BaseHoodieWriteClient#1209`), so they
serialize. Either ordering is covered: if the table service wins,
`getContiguousCompletedWriteTimeline` cuts the base below the new pending
instant; if indexing wins, that instant is `> indexUptoInstant`. Either way it
lands in the catch-up range. Test passes.
*Inflight-at-schedule handling is correct in principle.*
`getContiguousCompletedWriteTimeline` (`BaseHoodieTimeline#206-213`) cuts the
base below the earliest pending instant, so an instant inflight at schedule
time is deliberately excluded from bootstrap and delegated to catch-up,
matching the RFC. That delegation is sound. Test passes.
So the base-instant selection and the no-holes rule are doing their job.
Everything converges on the single defect you identified: catch-up verifies
that a deltacommit exists rather than that it carries entries for the
partitions in the plan.
**One framing point on the RFC.** Its prose says catch-up must "check for
all completed commit instants after `t` to ensure **each of them added entries
per its indexing plan**", and it explicitly designates catch-up as the TOCTOU
correction: *"the inflight writer may not see an indexing request that was just
added and proceed to commit without that. We will correct this during indexing
action completion."* The step-by-step shorthand ("has corresponding deltacommit
then continue") is only equivalent to that when the writer obligation always
holds, which is exactly what TOCTOU breaks. So the mechanism the RFC assigned
to correct this race is the one currently waving it through — that seems worth
stating plainly in whatever fix we land.
Also, smaller and in the same area: `getInstantsToCatchup` computes the
catch-up range with `findInstantsBefore`, which compares requested time, while
the timeline is ordered by completion time
(`InstantComparatorV2#orderingComparator`). For a table-service instant that
completes late those disagree, so the computed range can miss it.
**On the fix:** I would rather we address this holistically than patch the
symptoms. Both root causes need to go — catch-up verifying plan content, and
the writer signal moving back to the pending `.indexing.requested` action per
the original design. Fixing only the first leaves writers coordinating through
an unlocked config field, which will keep producing variants of this. Note the
two are not fully independent: an in-flight writer still cannot see a request
scheduled after it built its metadata writer, so fix two narrows the window and
fix one closes what is left. That is the RFC's own two-layer structure.
I think the absence of any test holding a writer and the indexer in flight
together is a large part of how the drift went unnoticed — the existing
coverage runs the indexer to completion and then rewinds instants, which
exercises rollback handling but never the orderings that decide whether a
commit lands in a new partition. The suite is in #20246 if it is useful as a
starting point; happy to coordinate on the fix.
Agree the `getLazyMergedFileSlices(maxInstantTime)` fencing here is a step
in the right direction for the bootstrap snapshot, and that it does not close
either gap on its own.
--
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]
