test(amber): add HDFSRecordStorageSpec for the hdfs record-storage arm - #7100
Merged
Merged
Conversation
HDFSRecordStorage was the only member of the record-storage family with no spec (its three siblings are 85-100% covered), and the hdfs:// arm of SequentialRecordStorage.getStorage was likewise unexercised. The class builds its own Configuration internally, so a test cannot inject one. The spec instead gets to the Hadoop FileSystem cache first: the cache key is (scheme, authority, UGI) only -- Configuration contents are not part of it -- so calling FileSystem.get on an hdfs:// URI with fs.hdfs.impl set to WinutilsFreeLocalFileSystem, before constructing the class under test, makes the internal FileSystem.get resolve to that instance. No reflection, no NameNode, and a unique authority per test keeps the cache entries isolated. The shim delegates to Hadoop's default local filesystem off Windows, so this runs unchanged in CI. Covers the scheme guard, the auto-create-folder branch, writer/reader round-tripping through the production serde, the EmptyRecordStorage fallback for an absent file, deleteStorage present and absent, containsFolder for a directory vs a plain file vs nothing, and the factory dispatch for both hdfs:// and HDFS://. Also updates the stale paragraph in SequentialRecordStorageSpec that claimed the hdfs arm was untestable and that regressions there would surface in higher-level suites -- no suite uses an hdfs:// URI.
Contributor
Automated Reviewer SuggestionsBased on the
|
Contributor
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🔴 | bs=10 sw=10 sl=64 | 405 | 0.247 | 23,971/29,319/29,319 us | 🟢 -13.7% / 🔴 +94.8% |
| ⚪ | bs=100 sw=10 sl=64 | 797 | 0.486 | 122,670/157,275/157,275 us | ⚪ within ±5% / 🔴 +46.5% |
| 🔴 | bs=1000 sw=10 sl=64 | 910 | 0.556 | 1,093,876/1,235,072/1,235,072 us | 🔴 +9.1% / 🔴 +20.2% |
Baseline details
Latest main f28d8ec from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 405 tuples/sec | 443 tuples/sec | 786.12 tuples/sec | -8.6% | -48.5% |
| bs=10 sw=10 sl=64 | MB/s | 0.247 MB/s | 0.271 MB/s | 0.48 MB/s | -8.9% | -48.5% |
| bs=10 sw=10 sl=64 | p50 | 23,971 us | 22,809 us | 12,305 us | +5.1% | +94.8% |
| bs=10 sw=10 sl=64 | p95 | 29,319 us | 33,970 us | 15,774 us | -13.7% | +85.9% |
| bs=10 sw=10 sl=64 | p99 | 29,319 us | 33,970 us | 18,978 us | -13.7% | +54.5% |
| bs=100 sw=10 sl=64 | throughput | 797 tuples/sec | 818 tuples/sec | 999.71 tuples/sec | -2.6% | -20.3% |
| bs=100 sw=10 sl=64 | MB/s | 0.486 MB/s | 0.499 MB/s | 0.61 MB/s | -2.6% | -20.4% |
| bs=100 sw=10 sl=64 | p50 | 122,670 us | 118,550 us | 100,616 us | +3.5% | +21.9% |
| bs=100 sw=10 sl=64 | p95 | 157,275 us | 150,568 us | 107,356 us | +4.5% | +46.5% |
| bs=100 sw=10 sl=64 | p99 | 157,275 us | 150,568 us | 113,255 us | +4.5% | +38.9% |
| bs=1000 sw=10 sl=64 | throughput | 910 tuples/sec | 912 tuples/sec | 1,031 tuples/sec | -0.2% | -11.8% |
| bs=1000 sw=10 sl=64 | MB/s | 0.556 MB/s | 0.557 MB/s | 0.63 MB/s | -0.2% | -11.7% |
| bs=1000 sw=10 sl=64 | p50 | 1,093,876 us | 1,094,906 us | 980,328 us | -0.1% | +11.6% |
| bs=1000 sw=10 sl=64 | p95 | 1,235,072 us | 1,132,422 us | 1,027,528 us | +9.1% | +20.2% |
| bs=1000 sw=10 sl=64 | p99 | 1,235,072 us | 1,132,422 us | 1,054,298 us | +9.1% | +17.1% |
Raw CSV
config_idx,batch_size,schema_width,string_len,num_batches,total_ms,total_tuples,total_bytes,tuples_per_sec,mb_per_sec,lat_p50_us,lat_p95_us,lat_p99_us
0,10,10,64,20,493.83,200,128000,405,0.247,23971.17,29319.48,29319.48
1,100,10,64,20,2509.29,2000,1280000,797,0.486,122670.44,157275.31,157275.31
2,1000,10,64,20,21968.61,20000,12800000,910,0.556,1093876.23,1235071.83,1235071.83
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7100 +/- ##
============================================
+ Coverage 79.39% 79.43% +0.04%
- Complexity 3801 3814 +13
============================================
Files 1159 1159
Lines 46150 46150
Branches 5129 5129
============================================
+ Hits 36641 36661 +20
+ Misses 7888 7866 -22
- Partials 1621 1623 +2
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Adds unit tests for the hdfs:// record-storage path in Amber by introducing a dedicated spec for HDFSRecordStorage and updating existing commentary to reflect that the hdfs factory-dispatch arm is now covered.
Changes:
- Added
HDFSRecordStorageSpecto exerciseHDFSRecordStoragebehaviors andSequentialRecordStorage.getStoragedispatch forhdfs:///HDFS://. - Updated
SequentialRecordStorageSpecdocumentation comment to pointhdfs://coverage at the new spec (instead of claiming it is untestable / indirectly covered elsewhere).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| amber/src/test/scala/org/apache/texera/amber/engine/common/storage/SequentialRecordStorageSpec.scala | Updates the factory-dispatch commentary to reflect that hdfs:// coverage lives in HDFSRecordStorageSpec. |
| amber/src/test/scala/org/apache/texera/amber/engine/common/storage/HDFSRecordStorageSpec.scala | New spec covering constructor scheme guard, auto-create-folder behavior, writer/reader round-trips, missing-file fallback, delete/idempotency, containsFolder semantics, and factory dispatch for hdfs:// / HDFS://. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this PR?
HDFSRecordStoragewas the only member of the record-storage family without a spec, and thehdfs://arm ofSequentialRecordStorage.getStoragewas unexercised too:EmptyRecordStorageSequentialRecordStorageNone/file://arms)hdfs://armVFSRecordStorageHDFSRecordStorageSequentialRecordStorageSpechad explicitly given up on it — "thehdfs://branch is unit-test-hostile (HDFSRecordStorage's constructor callsFileSystem.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
Configurationinternally, so a test cannot inject one. But Hadoop'sFileSystemcache is keyed on(scheme, authority, UGI)only — theConfiguration's contents are not part of the key. So the spec gets there first:That is the ordinary supported
FileSystem.getpath — 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 itsFileSystemin afinally(which evicts only its own key, leaving the JVM-global cache as found).WinutilsFreeLocalFileSystemis required, not incidental: plainLocalFileSystemdies at the constructor's firstmkdirson 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
EmptyRecordStoragefallback for an absent file (asserted empty, not assumed);deleteStorageboth present and absent;containsFolderfor a directory vs a plain file vs nothing (the plain-file case is what anexists-only implementation gets wrong); and factory dispatch for bothhdfs://andHDFS://(which pins both.toLowerCasecall sites).Also in this PR: the stale paragraph in
SequentialRecordStorageSpecis corrected to point at the new spec. Its second claim was never true — it said anhdfsregression "would surface immediately in higher-level checkpoint / fault-tolerance suites that use hdfs:// URIs", but no suite in the repo uses anhdfs://URI.Two branches are left uncovered on purpose, noted in-file: a scheme-less URI NPEs on
getScheme.toLowerCasebefore the assert evaluates (a latent production nit, not behaviour to cement), and thedfs.client…replace-datanode-on-failureconfig is unobservable without a real DataNode write pipeline.Unlike its siblings, this spec restores
AmberRuntime's_actorSystem/_serdeinafterAllinstead of nulling them, and builds itsActorSystemlazily. sbt runs amber's suites concurrently in one JVM and several install their own serde into those same globals; nulling makes a sibling'sAmberRuntime.serdetake its lazy-bootstrap branch mid-run and spin up an untracked clusterActorSystem, and a strictvalleaks one whenever a filter selects no test from the suite (asAMBER_TEST_FILTERdoes 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
AmberRuntimesave/restore change does not disturb them — 43 tests total, Java 17:Verified locally on Windows with no
winutils.exeand nohadoop.dll— the run logs Hadoop's ownDid not find winutils.exeprobe at WARN, which is exactly the failure plainLocalFileSystemwould have thrown.Test/scalafmtCheckandTest/scalafix --checkboth[success]. No production file is touched.Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 5)