fix(workflow-core): run Iceberg local storage on Windows without winutils#6488
Conversation
Automated Reviewer SuggestionsBased on the
|
Codecov Report❌ Patch coverage is ❌ Your patch status has failed because the patch coverage (41.66%) is below the target coverage (60.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #6488 +/- ##
============================================
- Coverage 70.44% 70.43% -0.01%
- Complexity 3404 3405 +1
============================================
Files 1142 1143 +1
Lines 44876 44887 +11
Branches 4955 4959 +4
============================================
+ Hits 31613 31618 +5
- Misses 11619 11623 +4
- Partials 1644 1646 +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:
|
There was a problem hiding this comment.
Pull request overview
This PR fixes workflow execution on Windows dev machines that don’t have a native Hadoop/winutils installation by ensuring Iceberg’s local-warehouse writes don’t fail on Hadoop’s LocalFileSystem chmod/chown path.
Changes:
- Add a Windows-gated
LocalFileSystemshim that skips permission/owner operations when winutils is unavailable. - Update
IcebergUtilto supply a HadoopConfigurationthat selects the shim viafs.file.impl(and disables the scheme-keyedFileSystemcache to prevent a previously cached default FS from bypassing the override). - Add regression tests covering both the catalog path (
createHadoopCatalog) and direct selection/behavior of the new file system.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| common/workflow-core/src/main/scala/org/apache/texera/amber/util/WinutilsFreeLocalFileSystem.scala | Introduces a LocalFileSystem/RawLocalFileSystem wrapper that skips chmod/chown on Windows when winutils is missing. |
| common/workflow-core/src/main/scala/org/apache/texera/amber/util/IcebergUtil.scala | Routes local Iceberg catalogs through a Windows-safe Hadoop Configuration when needed (otherwise unchanged). |
| common/workflow-core/src/test/scala/org/apache/texera/amber/util/WinutilsFreeLocalFileSystemSpec.scala | Tests selecting the FS via fs.file.impl and basic mkdir/create/read/delete plus setPermission no-throw. |
| common/workflow-core/src/test/scala/org/apache/texera/amber/util/IcebergUtilSpec.scala | Adds regression test ensuring createHadoopCatalog can create/load a table in a local warehouse across platforms. |
Comments suppressed due to low confidence (1)
common/workflow-core/src/main/scala/org/apache/texera/amber/util/IcebergUtil.scala:86
- The Scaladoc says this uses an “empty Hadoop Configuration”, but the implementation may now set
fs.file.impl/ disable the FileSystem cache on Windows hosts without winutils. This comment should be updated so future readers don’t assume the configuration is always empty.
* - Uses an empty Hadoop `Configuration`, meaning the local file system (or `file:/`) will be used by default
* instead of HDFS.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🔴 | bs=10 sw=10 sl=64 | 378 | 0.23 | 25,834/34,167/34,167 us | 🔴 +13.7% / 🔴 +137.9% |
| ⚪ | bs=100 sw=10 sl=64 | 792 | 0.483 | 124,855/150,295/150,295 us | ⚪ within ±5% / 🔴 +51.6% |
| ⚪ | bs=1000 sw=10 sl=64 | 900 | 0.549 | 1,113,612/1,140,534/1,140,534 us | ⚪ within ±5% / 🔴 +22.4% |
Baseline details
Latest main 1366b87 from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 378 tuples/sec | 415 tuples/sec | 831.47 tuples/sec | -8.9% | -54.5% |
| bs=10 sw=10 sl=64 | MB/s | 0.23 MB/s | 0.253 MB/s | 0.507 MB/s | -9.1% | -54.7% |
| bs=10 sw=10 sl=64 | p50 | 25,834 us | 22,726 us | 11,844 us | +13.7% | +118.1% |
| bs=10 sw=10 sl=64 | p95 | 34,167 us | 33,791 us | 14,359 us | +1.1% | +137.9% |
| bs=10 sw=10 sl=64 | p99 | 34,167 us | 33,791 us | 18,888 us | +1.1% | +80.9% |
| bs=100 sw=10 sl=64 | throughput | 792 tuples/sec | 808 tuples/sec | 1,089 tuples/sec | -2.0% | -27.3% |
| bs=100 sw=10 sl=64 | MB/s | 0.483 MB/s | 0.493 MB/s | 0.665 MB/s | -2.0% | -27.3% |
| bs=100 sw=10 sl=64 | p50 | 124,855 us | 122,068 us | 92,503 us | +2.3% | +35.0% |
| bs=100 sw=10 sl=64 | p95 | 150,295 us | 146,556 us | 99,121 us | +2.6% | +51.6% |
| bs=100 sw=10 sl=64 | p99 | 150,295 us | 146,556 us | 112,422 us | +2.6% | +33.7% |
| bs=1000 sw=10 sl=64 | throughput | 900 tuples/sec | 899 tuples/sec | 1,117 tuples/sec | +0.1% | -19.4% |
| bs=1000 sw=10 sl=64 | MB/s | 0.549 MB/s | 0.549 MB/s | 0.682 MB/s | 0.0% | -19.5% |
| bs=1000 sw=10 sl=64 | p50 | 1,113,612 us | 1,115,285 us | 910,046 us | -0.2% | +22.4% |
| bs=1000 sw=10 sl=64 | p95 | 1,140,534 us | 1,143,059 us | 950,716 us | -0.2% | +20.0% |
| bs=1000 sw=10 sl=64 | p99 | 1,140,534 us | 1,143,059 us | 985,832 us | -0.2% | +15.7% |
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,529.61,200,128000,378,0.230,25833.50,34166.89,34166.89
1,100,10,64,20,2525.28,2000,1280000,792,0.483,124854.94,150295.46,150295.46
2,1000,10,64,20,22233.56,20000,12800000,900,0.549,1113611.82,1140533.94,1140533.94
|
@aglinxinyuan can we take the chance to fill up the unit test as well? do we also need to add a windows CI to prevent this from happening again? |
The unit test is there. Which part is missing? |
CI did not alert us when upgrading the Hadoop dependency. "the timeline pins it to the early-July dependency bumps. Windows dev worked on hadoop-common 3.3.1; after #6201 (3.4.3) and #6227 (3.5.0) every execution crashes at its first local Iceberg write" Just want to prevent this from happening again. also #6488 (comment) can we take the chance to fill up those coverage? |
…age of both winutils gate branches, setOwner test)
Done in f68aad0 — the winutils gate in
Agreed — that's the only thing that would have flagged #6201/#6227. Opened #6503 to track a minimal |
|
ah, do we need this fix in v1.2? @aglinxinyuan @xuang7 |
Do we release the source code? I guess this error doesn't happen on Docker images? |
|
Technically, we release both images and source code (release is tagged on commit). But this is not blocking RC3. We should anyway backport it to v1.2 branch, so a future RC4 or 1.2.1 can pick it up if necessary. |
What changes were proposed in this PR?
On a Windows dev machine without a native Hadoop installation, every workflow execution fails at execution start when the
postgresorhadoopIceberg catalog type is configured. Hadoop's local file system applies POSIX permissions on every file/directory creation by shelling out to%HADOOP_HOME%\bin\winutils.exe, with no configuration flag to disable it — and Iceberg'sHadoopFileIO(the local warehouse used by thepostgresandhadoopcatalog types) writes through exactly that path. The stock default catalog isrest(#6049), which writes throughS3FileIOand is unaffected.This is a regression from the CVE-driven hadoop-common bumps: Windows dev worked on 3.3.1; after #6201 (3.4.3) and #6227 (3.5.0), 3.4+ applies POSIX permissions on every local file/directory creation, which pulls winutils in:
POSIX permission bits carry no meaning on NTFS, so this PR skips them there instead of failing:
WinutilsFreeLocalFileSystem.scala(new)LocalFileSystemwhosesetPermission/setOwnerskip the winutils shell-out. Self-gating: when winutils is installed, or on non-Windows hosts, it delegates to Hadoop's default behavior.IcebergUtil.scalanewLocalHadoopConf()used bycreateHadoopCatalog/createPostgresCatalog. Only whenShell.WINDOWS && !Shell.hasWinutilsPath()does it select the new file system viafs.file.impl(and disable the scheme-keyedFileSystemcache so an earlier cached instance cannot bypass the override). On all other hosts it returns the defaultConfigurationunchanged.Before → after:
postgres/hadoopcatalog, no winutilsrestcatalog (stock default)S3FileIO)Any related issues, documentation, discussions?
Closes #6487
Related: #6150 (possible removal of the
hadoopcatalog type — the fix targetsHadoopFileIO, which thepostgrescatalog also uses, so it is needed independently; the regression test currently drivesHadoopFileIOthroughcreateHadoopCatalogas a lightweight vehicle and can be re-anchored on the local file system directly if thehadooptype is removed)How was this PR tested?
IcebergUtilSpec(create + load a table viacreateHadoopCatalogin a temp local warehouse). Written first against stockmainon a Windows machine without winutils: it fails with the exact production error (RawLocalFileSystem.setPermission → Shell.getSetPermissionCommand → FileNotFoundException: Hadoop bin directory does not exist), and passes with this fix. On Linux CI it exercises the unchanged default path.newLocalHadoopConf's winutils gate is injectable (defaults to the host's real winutils availability), so both branches — including the Windows-only override — are unit-tested on every platform: selection ofWinutilsFreeLocalFileSystemviafs.file.impl, and the untouched-Configurationpath.WinutilsFreeLocalFileSystemSpec: selection throughfs.file.impl(same mechanism production uses),mkdirs/create/read/delete round-trip, andsetPermission/setOwnerno-throw.postgrescatalog previously failed atExecutionStatsServicetable creation and now succeeds end-to-end (JdbcCatalog+HadoopFileIO→ local warehouse write).WorkflowCore/scalafmtCheckAllandWorkflowCore/scalafixAll --checkpass.Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Fable 5)