Skip to content

fix(pyamber, v1.3): preserve channel marker ports - #8621

Merged
mengw15 merged 1 commit into
apache:release/v1.3from
mengw15:backport/8432-preserve-channel-marker-ports-v1.3
Sep 21, 2026
Merged

mengw15 merged 1 commit into
apache:release/v1.3from
mengw15:backport/8432-preserve-channel-marker-ports-v1.3

Conversation

@mengw15

@mengw15 mengw15 commented Sep 21, 2026

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

Backport of #8432 to release/v1.3: a clean cherry-pick of its squash commit, no adaptations — the backport commit reuses the squash message and author, as the automated fast path would. See #8432 for the change itself (channel markers bind to the port resolved by their control handler instead of the last data tuple's port, and invalid markers are rejected before entering the executor).

Opened manually by the v1.3 release manager: the automated fast path cherry-picked this cleanly and then pushed it straight to release/v1.3, where the Merge Queue ruleset rejected the push (GH013, run 35187408300). The Actions-app bypass meant to unblock that path (#8379) was never created — asfyaml rejects an Integration bypass actor — and the failing job's notification 403s for want of pull-requests: write, so the loss left neither a backport PR nor a comment on #8432. See #8377.

Source: 471e53c

Any related issues, documentation, discussions?

Backport of #8432. Related to #8319.

How was this PR tested?

The change is identical to #8432, which carries 235 lines of new pytest coverage (test_channel_marker_handlers.py, test_data_processor.py, test_main_loop.py); the backport tree is verified byte-identical to cherry-picking the squash commit onto release/v1.3, and release-branch CI runs the full matrix on this PR. The two branch-level blockers that used to make pyamber jobs red here are already merged: #8549 (LICENSE-binary-python drift) and #8550 (RustFS as the default object store).

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

Yes. Generated-by: Claude Code (claude-opus-5)

### What changes were proposed in this PR?

Bind channel markers to the input port resolved by their control
handler. Lifecycle callbacks use that port instead of the last data
tuple's port. Reject invalid markers before entering the executor.

### Any related issues, documentation, discussions?

Related to apache#8319.

### How was this PR tested?

From amber, on Python 3.12:

```text
python -m pytest --tb=short -q src/test/python/core/architecture/handlers/control/test_channel_marker_handlers.py src/test/python/core/runnables/test_data_processor.py src/test/python/core/runnables/test_main_loop.py
66 passed
python -m ruff check src/main/python src/test/python
All checks passed
python -m ruff format --check src/main/python src/test/python
215 files already formatted
```

The new handler-to-processor cases cover an empty port 1 finishing after
a tuple on port 0, and port 1 starting before any tuple. Both fail when
marker routing is reverted to the current data port. Invalid-marker
tests failed before adding the type guard.

The full run, `python -m pytest --tb=no -q`, finished with 1320 passed,
1 xfailed, and 1 failed. The failure is the unchanged
`test_rest_catalog_round_trip` integration test, which targets a
Lakekeeper service at localhost:8181. The full suite is not green.
Excluding integration tests, `python -m pytest --tb=no -q -m "not
integration"` passes: 1320 passed, 1 deselected, 1 xfailed.

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

Generated-by: OpenAI Codex

(backported from commit 471e53c)

Co-authored-by: Meng Wang <mengw15@uci.edu>
@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: @Yicong-Huang, @aglinxinyuan
    You can notify them by mentioning @Yicong-Huang, @aglinxinyuan in a comment.

@mengw15
mengw15 requested a review from xuang7 September 21, 2026 07:01
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.06%. Comparing base (5036097) to head (2f9c4e7).

Additional details and impacted files
@@               Coverage Diff               @@
##             release/v1.3    #8621   +/-   ##
===============================================
  Coverage           94.06%   94.06%           
  Complexity           4808     4808           
===============================================
  Files                1194     1194           
  Lines               48601    48611   +10     
  Branches             5860     5862    +2     
===============================================
+ Hits                45718    45728   +10     
  Misses               1429     1429           
  Partials             1454     1454           
Flag Coverage Δ *Carryforward flag
access-control-service 81.00% <ø> (ø) Carriedforward from 5036097
agent-service 99.32% <ø> (ø) Carriedforward from 5036097
amber 90.10% <ø> (ø) Carriedforward from 5036097
computing-unit-managing-service 73.67% <ø> (ø) Carriedforward from 5036097
config-service 87.00% <ø> (ø) Carriedforward from 5036097
file-service 87.75% <ø> (ø) Carriedforward from 5036097
frontend 96.81% <ø> (ø) Carriedforward from 5036097
notebook-migration-service 79.31% <ø> (ø) Carriedforward from 5036097
pyamber 98.18% <100.00%> (+<0.01%) ⬆️
workflow-compiling-service 77.19% <ø> (ø) Carriedforward from 5036097

*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

Copy link
Copy Markdown
Contributor

⚠️ Benchmark changes need a look

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

Compared against main 5036097 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 363 0.222 25,903/35,881/35,881 us 🔴 +9.9% / 🔴 +127.1%
🟢 bs=100 sw=10 sl=64 800 0.488 124,941/141,801/141,801 us 🟢 -6.4% / 🔴 +30.4%
bs=1000 sw=10 sl=64 913 0.557 1,088,132/1,193,053/1,193,053 us ⚪ within ±5% / 🔴 +15.0%
Baseline details

Latest main 5036097 from same runner

config metric PR latest main 7d avg Δ latest Δ 7d
bs=10 sw=10 sl=64 throughput 363 tuples/sec 391 tuples/sec 767.43 tuples/sec -7.2% -52.7%
bs=10 sw=10 sl=64 MB/s 0.222 MB/s 0.238 MB/s 0.468 MB/s -6.7% -52.6%
bs=10 sw=10 sl=64 p50 25,903 us 23,566 us 12,880 us +9.9% +101.1%
bs=10 sw=10 sl=64 p95 35,881 us 36,553 us 15,801 us -1.8% +127.1%
bs=10 sw=10 sl=64 p99 35,881 us 36,553 us 19,767 us -1.8% +81.5%
bs=100 sw=10 sl=64 throughput 800 tuples/sec 817 tuples/sec 988.73 tuples/sec -2.1% -19.1%
bs=100 sw=10 sl=64 MB/s 0.488 MB/s 0.499 MB/s 0.603 MB/s -2.2% -19.1%
bs=100 sw=10 sl=64 p50 124,941 us 121,423 us 102,684 us +2.9% +21.7%
bs=100 sw=10 sl=64 p95 141,801 us 151,454 us 108,712 us -6.4% +30.4%
bs=100 sw=10 sl=64 p99 141,801 us 151,454 us 118,731 us -6.4% +19.4%
bs=1000 sw=10 sl=64 throughput 913 tuples/sec 922 tuples/sec 1,022 tuples/sec -1.0% -10.7%
bs=1000 sw=10 sl=64 MB/s 0.557 MB/s 0.563 MB/s 0.624 MB/s -1.1% -10.7%
bs=1000 sw=10 sl=64 p50 1,088,132 us 1,087,929 us 999,086 us +0.0% +8.9%
bs=1000 sw=10 sl=64 p95 1,193,053 us 1,137,583 us 1,037,033 us +4.9% +15.0%
bs=1000 sw=10 sl=64 p99 1,193,053 us 1,137,583 us 1,066,123 us +4.9% +11.9%
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,550.58,200,128000,363,0.222,25902.83,35880.75,35880.75
1,100,10,64,20,2499.20,2000,1280000,800,0.488,124941.47,141801.45,141801.45
2,1000,10,64,20,21907.96,20000,12800000,913,0.557,1088132.17,1193052.57,1193052.57

@xuang7 xuang7 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mengw15
mengw15 added this pull request to the merge queue Sep 21, 2026
Merged via the queue into apache:release/v1.3 with commit 1736427 Sep 21, 2026
26 checks passed
@mengw15
mengw15 deleted the backport/8432-preserve-channel-marker-ports-v1.3 branch September 21, 2026 20: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.

4 participants