The GitHub Actions job "Required Checks" on texera.git/gh-readonly-queue/main/pr-7100-fe9c4900234f535ae80fb002138f8c9386e68bb8 has succeeded. Run started by GitHub user aglinxinyuan (triggered by aglinxinyuan).
Head commit for run: 4e152e0d823224a42a59ade8abbd1f2c9075ca11 / Xinyuan Lin <[email protected]> test(amber): add HDFSRecordStorageSpec for the hdfs record-storage arm (#7100) ### What changes were proposed in this PR? `HDFSRecordStorage` was the only member of the record-storage family without a spec, and the `hdfs://` arm of `SequentialRecordStorage.getStorage` was unexercised too: | Class | Spec before | Spec now | |---|---|---| | `EmptyRecordStorage` | ✅ | ✅ | | `SequentialRecordStorage` | ✅ (`None` / `file://` arms) | ✅ + `hdfs://` arm | | `VFSRecordStorage` | ✅ | ✅ | | `HDFSRecordStorage` | ❌ **none** | ✅ **new** | `SequentialRecordStorageSpec` had explicitly given up on it — "the `hdfs://` branch is unit-test-hostile (`HDFSRecordStorage`'s constructor calls `FileSystem.get`, which can block on DNS / network)". It turns out to be testable with no NameNode and no native Hadoop. **How.** The class builds its own `Configuration` internally, so a test cannot inject one. But Hadoop's `FileSystem` cache is keyed on `(scheme, authority, UGI)` **only** — the `Configuration`'s contents are not part of the key. So the spec gets there first: ``` test: FileSystem.get("hdfs://<uuid>:9000/", conf) conf: fs.hdfs.impl = WinutilsFreeLocalFileSystem │ └─▶ populates FileSystem.CACHE under (hdfs, <uuid>:9000, ugi) ▼ prod: new HDFSRecordStorage(uri) ─▶ FileSystem.get(uri, new Configuration()) └─▶ cache hit: same instance, fs.hdfs.impl never re-resolved, no NameNode contacted ``` That is the ordinary supported `FileSystem.get` path — **no reflection anywhere**. A unique authority per test means a unique cache key per test, so tests cannot see each other's instance or working directory, and each closes its `FileSystem` in a `finally` (which evicts only its own key, leaving the JVM-global cache as found). `WinutilsFreeLocalFileSystem` is required, not incidental: plain `LocalFileSystem` dies at the constructor's first `mkdirs` on a winutils-less Windows host (`RawLocalFileSystem.setPermission` → `Shell.getWinUtilsPath` → *"Hadoop bin directory does not exist"*). The shim skips permission ops only on that host and otherwise delegates to Hadoop's default local filesystem, so this spec runs unchanged on Linux/macOS CI. Covered: the scheme guard; the auto-create-folder branch; writer/reader round-tripping through the production serde; distinct files kept independent; re-opened writer overwriting rather than appending; the `EmptyRecordStorage` fallback for an absent file (asserted empty, not assumed); `deleteStorage` both present and absent; `containsFolder` for a directory vs a plain file vs nothing (the plain-file case is what an `exists`-only implementation gets wrong); and factory dispatch for both `hdfs://` and `HDFS://` (which pins both `.toLowerCase` call sites). Also in this PR: the stale paragraph in `SequentialRecordStorageSpec` is corrected to point at the new spec. Its second claim was never true — it said an `hdfs` regression "would surface immediately in higher-level checkpoint / fault-tolerance suites that use hdfs:// URIs", but no suite in the repo uses an `hdfs://` URI. Two branches are left uncovered on purpose, noted in-file: a scheme-less URI NPEs on `getScheme.toLowerCase` before the assert evaluates (a latent production nit, not behaviour to cement), and the `dfs.client…replace-datanode-on-failure` config is unobservable without a real DataNode write pipeline. Unlike its siblings, this spec **restores** `AmberRuntime`'s `_actorSystem` / `_serde` in `afterAll` instead of nulling them, and builds its `ActorSystem` lazily. sbt runs amber's suites concurrently in one JVM and several install their own serde into those same globals; nulling makes a sibling's `AmberRuntime.serde` take its lazy-bootstrap branch mid-run and spin up an untracked cluster `ActorSystem`, and a strict `val` leaks one whenever a filter selects no test from the suite (as `AMBER_TEST_FILTER` does in the integration job). ### Any related issues, documentation, discussions? Closes #7098 ### How was this PR tested? One new spec, 14 tests. Run together with all three existing sibling specs to confirm the `AmberRuntime` save/restore change does not disturb them — 43 tests total, Java 17: ``` sbt "WorkflowExecutionService/testOnly org.apache.texera.amber.engine.common.storage.HDFSRecordStorageSpec org.apache.texera.amber.engine.common.storage.SequentialRecordStorageSpec org.apache.texera.amber.engine.common.storage.VFSRecordStorageSpec org.apache.texera.amber.engine.common.storage.EmptyRecordStorageSpec" ``` ``` [info] Suites: completed 4, aborted 0 [info] Tests: succeeded 43, failed 0, canceled 0, ignored 0, pending 0 [info] All tests passed. ``` Verified locally on Windows with **no `winutils.exe` and no `hadoop.dll`** — the run logs Hadoop's own `Did not find winutils.exe` probe at WARN, which is exactly the failure plain `LocalFileSystem` would have *thrown*. `Test/scalafmtCheck` and `Test/scalafix --check` both `[success]`. No production file is touched. ### Was this PR authored or co-authored using generative AI tooling? Generated-by: Claude Code (Opus 5) Report URL: https://github.com/apache/texera/actions/runs/30511882814 With regards, GitHub Actions via GitBox
