Skip to content

feat(pytexera): stop writes after upload failure - #8260

Open
carloea2 wants to merge 1 commit into
apache:mainfrom
carloea2:fix/stop-binary-write-on-upload-failure
Open

feat(pytexera): stop writes after upload failure#8260
carloea2 wants to merge 1 commit into
apache:mainfrom
carloea2:fix/stop-binary-write-on-upload-failure

Conversation

@carloea2

@carloea2 carloea2 commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

Use timed queue writes so a producer notices when the background upload has failed. Stop waiting to enqueue the EOF marker once that upload worker has completed.

A regression covers both write and close with a full queue after an upload failure.

Any related issues, documentation, discussions?

Closes #8259

How was this PR tested?

Live reproduction on the untouched base reported write_still_blocked=True. The same probe after the fix reported write_still_blocked=False and surfaced the upload error.

C:\Users\carlo\texera\texera\.venv312\Scripts\python.exe -c "import sys,pytest; sys.path[:0]=[r'C:\Users\carlo\texera\texera-worktrees\investigate-bug66\amber\src\main\python',r'C:\Users\carlo\texera\texera\amber\src\main\python']; raise SystemExit(pytest.main([r'amber/src/test/python/pytexera/storage/test_large_binary_output_stream.py','-q','-p','no:cacheprovider']))"

C:\Users\carlo\texera\texera\.venv312\Scripts\ruff.exe check amber/src/main/python amber/src/test/python

C:\Users\carlo\texera\texera\.venv312\Scripts\ruff.exe format --check amber/src/main/python/pytexera/storage/large_binary_output_stream.py amber/src/test/python/pytexera/storage/test_large_binary_output_stream.py

Was this PR authored or co-authored using generative AI tooling?

Generated-by: Codex

@Yicong-Huang Yicong-Huang added the release/v1.2 back porting to release/v1.2 label Aug 31, 2026
@github-actions
github-actions Bot requested a review from xuang7 August 31, 2026 05:45
@github-actions

Copy link
Copy Markdown
Contributor

Backport auto-label report

This fix: PR was checked against each actively-supported release branch. release/* labels drive the post-merge backport, so add or remove one to change where this fix lands.

Release branch Analysis
⚠️ release/v1.3 Not labeled automatically — none of the files this PR modifies exist on this branch (amber/src/main/python/pytexera/storage/large_binary_output_stream.py, amber/src/test/python/pytexera/storage/test_large_binary_output_stream.py). The fix may target code that isn't on this release, or the files were moved/renamed after the branch was cut. Please check and add release/v1.3 by hand if this fix should be backported here.
release/v1.2 Change detected on this branch — label added; this fix is queued to backport here. Requested review from @xuang7.

Auto-label run.

@github-actions

Copy link
Copy Markdown
Contributor

Automated Reviewer Suggestions

Based on the git blame history of the changed files, we recommend the following reviewers:

  • Contributors with relevant context: @kunwp1
    You can notify them by mentioning @kunwp1 in a comment.

@codecov-commenter

codecov-commenter commented Aug 31, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 56.25000% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.85%. Comparing base (50321e4) to head (9038c71).
⚠️ Report is 11 commits behind head on main.

Files with missing lines Patch % Lines
...hon/pytexera/storage/large_binary_output_stream.py 56.25% 5 Missing and 2 partials ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #8260      +/-   ##
============================================
- Coverage     93.93%   93.85%   -0.08%     
  Complexity     4762     4762              
============================================
  Files          1191     1191              
  Lines         48308    48322      +14     
  Branches       5377     5827     +450     
============================================
- Hits          45380    45355      -25     
- Misses         1494     1499       +5     
- Partials       1434     1468      +34     
Flag Coverage Δ *Carryforward flag
access-control-service 81.00% <ø> (ø) Carriedforward from 50321e4
agent-service 99.32% <ø> (ø) Carriedforward from 50321e4
amber 90.03% <ø> (ø) Carriedforward from 50321e4
computing-unit-managing-service 73.67% <ø> (ø) Carriedforward from 50321e4
config-service 86.86% <ø> (ø) Carriedforward from 50321e4
file-service 87.91% <ø> (ø) Carriedforward from 50321e4
frontend 96.41% <ø> (ø) Carriedforward from 50321e4
notebook-migration-service 79.31% <ø> (ø) Carriedforward from 50321e4
pyamber 98.03% <56.25%> (-0.84%) ⬇️
workflow-compiling-service 77.19% <ø> (ø) Carriedforward from 50321e4

*This pull request uses carry forward flags. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions

github-actions Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

⚠️ Benchmark changes need a look

🟢 2 better · 🔴 5 worse · ⚪ 8 noise (<±5%) · 0 without baseline

Compared against main 7d57cd6 benchmarked on this same runner, so the delta is largely free of cross-runner hardware noise. The "7d avg" column still reflects the gh-pages dashboard. Treat <±5% as noise unless repeated.

Dashboard · Run

config throughput MB/s latency max Δ latest / 7d
🔴 bs=10 sw=10 sl=64 529 0.323 17,336/28,019/28,019 us 🔴 +8.7% / 🔴 +79.8%
🟢 bs=100 sw=10 sl=64 1,161 0.709 85,276/101,177/101,177 us 🟢 -10.7% / 🟢 +15.3%
🔴 bs=1000 sw=10 sl=64 1,286 0.785 763,568/861,393/861,393 us 🔴 +8.2% / 🟢 +23.4%
Baseline details

Latest main 7d57cd6 from same runner

config metric PR latest main 7d avg Δ latest Δ 7d
bs=10 sw=10 sl=64 throughput 529 tuples/sec 560 tuples/sec 787.98 tuples/sec -5.5% -32.9%
bs=10 sw=10 sl=64 MB/s 0.323 MB/s 0.342 MB/s 0.481 MB/s -5.6% -32.8%
bs=10 sw=10 sl=64 p50 17,336 us 15,944 us 12,593 us +8.7% +37.7%
bs=10 sw=10 sl=64 p95 28,019 us 27,368 us 15,579 us +2.4% +79.8%
bs=10 sw=10 sl=64 p99 28,019 us 27,368 us 18,786 us +2.4% +49.1%
bs=100 sw=10 sl=64 throughput 1,161 tuples/sec 1,133 tuples/sec 1,008 tuples/sec +2.5% +15.2%
bs=100 sw=10 sl=64 MB/s 0.709 MB/s 0.691 MB/s 0.615 MB/s +2.6% +15.3%
bs=100 sw=10 sl=64 p50 85,276 us 85,864 us 100,701 us -0.7% -15.3%
bs=100 sw=10 sl=64 p95 101,177 us 113,288 us 107,244 us -10.7% -5.7%
bs=100 sw=10 sl=64 p99 101,177 us 113,288 us 116,122 us -10.7% -12.9%
bs=1000 sw=10 sl=64 throughput 1,286 tuples/sec 1,322 tuples/sec 1,042 tuples/sec -2.7% +23.4%
bs=1000 sw=10 sl=64 MB/s 0.785 MB/s 0.807 MB/s 0.636 MB/s -2.7% +23.4%
bs=1000 sw=10 sl=64 p50 763,568 us 760,330 us 981,959 us +0.4% -22.2%
bs=1000 sw=10 sl=64 p95 861,393 us 795,973 us 1,023,080 us +8.2% -15.8%
bs=1000 sw=10 sl=64 p99 861,393 us 795,973 us 1,051,697 us +8.2% -18.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,377.89,200,128000,529,0.323,17336.29,28018.58,28018.58
1,100,10,64,20,1721.93,2000,1280000,1161,0.709,85275.84,101176.96,101176.96
2,1000,10,64,20,15551.46,20000,12800000,1286,0.785,763567.50,861393.45,861393.45

@Yicong-Huang

Copy link
Copy Markdown
Contributor

sounds like a feat. we never had this time-based stop before.

@carloea2
carloea2 force-pushed the fix/stop-binary-write-on-upload-failure branch from 9a9d599 to 9038c71 Compare August 31, 2026 20:19
@carloea2 carloea2 changed the title fix(pytexera): stop writes after upload failure feat(pytexera): stop writes after upload failure Aug 31, 2026
@carloea2

Copy link
Copy Markdown
Contributor Author

Changed the title and commit to feat. I cannot change repository labels, so fix and release/v1.2 still need to be replaced with feature.

@carloea2

Copy link
Copy Markdown
Contributor Author

Python 3.12 failed only on an unrelated test_multiple_producer_race timeout. 1298 tests passed, and Python 3.11 and 3.13 are green. I cannot rerun Actions on apache/texera.

@mengw15 mengw15 removed the release/v1.2 back porting to release/v1.2 label Aug 31, 2026
@xuang7 xuang7 removed the fix label Sep 1, 2026
@xuang7
xuang7 removed their request for review September 1, 2026 01:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Large binary writes can hang after a background upload failure

5 participants