apache / apache/hudi

[DISCUSS] HFile block cache assumes storage paths are immutable, but 1.x rollback + retry can rewrite a path in place

Open
#19,429 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Java
Stars
6.2k
Forks
2.5k
Avg merge
2d 8h
Merged PRs (30d)
111

Description

### Problem

The JVM-global HFile block cache (`hoodie.hfile.block.cache.enabled=true` by default since 1.1.0) keys blocks by `(filePath, offset, size)` with no content identity. That is only correct if a storage path never holds different bytes.

Hudi 1.x (table version 8 and above) does not guarantee that invariant:

1. Rollback **deletes** log files (no rollback command blocks anymore).
2. Table-service re-attempts reuse the **same instant time**.
3. On engines with deterministic write tokens (Java client is `0-0-0`; Spark falls back to it when `TaskContext` is null), the re-attempt's MDT log file gets the same `(fileId, instantTime, logVersion, writeToken)` name, so the same path is rewritten with different content.

A reader that cached blocks from the first incarnation then serves them for the second. The cache TTL is expire-after-access, so a repeatedly-read stale block never expires. Observable result: `getAllFilesInPartition` returns the rolled-back attempt's files and misses the re-attempt's files.

### Why this is currently latent, not live

MDT readers filter log blocks to instants completed on the data timeline (`getValidInstantTimestamps`). In the normal crash-and-retry flow the failed attempt's instant never completes, so its blocks are never read and never cached. The cache can only be poisoned when a **completed** instant is read, then unwound and re-attempted at the same instant time, e.g. manually removing a completed table-service instant to force a redo. So today the reader guards mask the violated invariant instead of the invariant being enforced.

### The design question: should Hudi restore file immutability?

The narrow fix is to add content identity to the cache key (`path + length + modificationTime` from `StoragePathInfo`); a patch exists and a regression test proves it. But that patches one consumer of the broken invariant.

The broader question for discussion: **should "a storage path, once written, is never rewritten with different content" be a contract Hudi guarantees by construction?** Restoring it means a re-attempt can never reuse a prior attempt's full file name, e.g. by making the write token unique per attempt on engines where it is currently deterministic. That would protect every component that assumes path identity (block caches, file-system view metadata caches, external caches such as S3 client-side caching or CDN-fronted storage), not just this one, and would eliminate the residual collision windows of the cache-key fix (same-length rewrite within filesystem mtime granularity; `StoragePathInfo` has no etag today).

Costs and open points of the immutability route:

- The write token format `partition-stage-attempt` is parsed numerically, so per-attempt uniqueness must stay in-format.
- Rollback-before-reattempt ordering becomes a hard invariant so two same-version, different-token files never coexist as valid.
- Covers MDT log appends, MDT compaction base files, and data-table compaction re-attempts.

### Proposal

1. Ship the content-aware cache key now (defense in depth; also covers path reuse Hudi's writers do not control, such as DR copy-back).
2. Discuss adopting path immutability as a 1.x storage-format contract, and if adopted, implement per-attempt write-token uniqueness.
3. Consider an etag/generation field on `StoragePathInfo` for true content identity on object stores.

### Appendix: how this was found

Flaky `TestJavaHoodieBackedMetadata#testReattemptOfFailedClusteringCommit`: the test simulates "clustering succeeded in MDT, failed before data-table commit" by completing the clustering, reading the MDT (which populates the cache while the instant is valid), and then deleting the completed replacecommit. The re-attempt rewrites the MDT log file at the same path and the next lookup serves the stale cached block. The Spark variant of the test passes only because Spark's task-context-derived write token differs across attempts, which changes the file name; the cache flaw is engine-independent. A regression test at the reader-factory level (`testBlockCacheNotReusedAfterFileRewrittenAtSamePath`) reproduces the stale read without any timeline involvement.

Contributor guide

No contributing guide indexed for this repository

Research direction

Start with the reader-factory regression test named testBlockCacheNotReusedAfterFileRewrittenAtSamePath and review the proposed content-aware cache key using StoragePathInfo. Then examine the write-token and rollback/reattempt behavior described for MDT and data-table files. Done means the chosen scope is agreed, the stale-cache regression is covered, and any immutability changes preserve the documented token format and ordering invariants.

Written by the indexing model from the issue text.

Assessment

Tech stack
java
Domain
data-engineering, distributed-systems, performance
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.