You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@texera.apache.org by "mengw15 (via GitHub)" <gi...@apache.org> on 2026/07/26 04:47:59 UTC
[PR] fix(pyamber): repair LinkedBlockingMultiQueue removal paths; add unit tests [texera]
mengw15 opened a new pull request, #6903:
URL: https://github.com/apache/texera/pull/6903
### What changes were proposed in this PR?
`LinkedBlockingMultiQueue` sits at ~70% line coverage, and writing tests for the untested parts surfaced two real defects — both on the removal paths, which is why those lines were never exercised. This fixes both and adds the tests that pin the corrected behavior.
**`SubQueue.remove` corrupted the chain and desynchronised the count.** `head` is a sentinel, so the candidate node is `trail.next`, but `remove` matched `trail.item` (the predecessor's own item) and then unlinked `trail.next`; `unlink` in turn cleared `trail.item` instead of the removed node's, and never decremented `self.count`. On `["a", "b", "c"]`, `remove("a")` returned `True` but left a `None` in place of `"a"` and dropped `"b"` entirely, left `SubQueue.size()` at 3 while the queue total went to 2, and made the next `get()` return `None` instead of an item. Fixed by matching `trail.next.item`, clearing `next_.item`, and decrementing `self.count` in `unlink`.
**`remove_sub_queue` raised `AttributeError` for any enabled sub-queue.** `add_sub_queue` constructed priority groups through class access (`LinkedBlockingMultiQueue.PriorityGroup(...)`), and the `@inner` descriptor only rebinds `owner` to the outer instance on instance access — so `PriorityGroup.owner` stayed the class and `self.owner.total_count.get_and_dec(...)` in `remove_queue` failed with `type object 'LinkedBlockingMultiQueue' has no attribute 'total_count'`. Fixed by constructing through `self.PriorityGroup(...)`, matching how `SubQueue` is already built. `Node` and `DefaultSubQueueSelection` are also built through class access but never read `self.owner`, so they are left unchanged.
Neither method is reachable from production code today — `internal_queue.py`, the only caller of this class, uses `put`/`get`/`size`/`enable`/`disable`/`is_empty`/`is_enabled`/`in_mem_size`/`add_sub_queue` — so no existing caller changes behavior.
The spec grows by 30 tests covering `peek` at all three levels, `SubQueue.clear`, the corrected removal paths, `PriorityGroup.remove_queue`, `remove_sub_queue`, and the small guards; the `queue` fixture moved to module scope so the new classes share it. Two current behaviors are pinned rather than changed: `clear()` does not reset `in_mem_size`, and `SubQueue.__str__` only supports `str` items.
### Any related issues, documentation, discussions?
Closes #6899.
### How was this PR tested?
The target file passes with 42 tests (12 existing, 30 new), and the full `pytest -m "not integration"` suite passes with no regressions; `ruff check` and `ruff format --check` are clean. Both defects were reproduced before the fix and confirmed gone after. For the failure path, each fix was reverted in turn: reverting the `remove` fix reddens 4 removal tests, and reverting the owner-binding fix brings back the original `AttributeError` — so the new tests genuinely guard both fixes.
### Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Fable 5)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(pyamber): repair LinkedBlockingMultiQueue removal paths; add unit tests [texera]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #6903:
URL: https://github.com/apache/texera/pull/6903#issuecomment-5082077355
<!-- texera-reviewer-suggestion -->
### 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(pyamber): repair LinkedBlockingMultiQueue removal paths; add unit tests [texera]
Posted by "Yicong-Huang (via GitHub)" <gi...@apache.org>.
Yicong-Huang commented on PR #6903:
URL: https://github.com/apache/texera/pull/6903#issuecomment-5126189127
Backport PR opened: #7101 (https://github.com/apache/texera/pull/7101) to `release/v1.2`, assigned to @mengw15 — part of a backport-coverage audit (clean cherry-pick).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(pyamber): repair LinkedBlockingMultiQueue removal paths; add unit tests [texera]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #6903:
URL: https://github.com/apache/texera/pull/6903#issuecomment-5082095758
<!-- texera-benchmarks-comment -->
## ⚠️ Benchmark changes need a look
🟢 0 better · 🔴 10 worse · ⚪ 5 noise (<±5%) · 0 without baseline
> Compared against main `4af3a85` 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](https://apache.github.io/texera/dev/bench/) · [Run](https://github.com/apache/texera/actions/runs/30188354606)
| | config | throughput | MB/s | latency | max Δ latest / 7d |
|---|---|---:|---:|---:|---:|
| 🔴 | bs=10 sw=10 sl=64 | 363 | 0.221 | 26,792/38,040/38,040 us | 🔴 -15.3% / 🔴 +129.2% |
| 🔴 | bs=100 sw=10 sl=64 | 776 | 0.473 | 128,596/161,670/161,670 us | 🔴 +21.6% / 🔴 +48.2% |
| ⚪ | bs=1000 sw=10 sl=64 | 904 | 0.552 | 1,105,721/1,196,679/1,196,679 us | ⚪ within ±5% / 🔴 +14.4% |
<details><summary>Baseline details</summary>
Latest main `4af3a85` from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---:|---:|---:|---:|---:|
| bs=10 sw=10 sl=64 | throughput | 363 tuples/sec | 427 tuples/sec | 754.55 tuples/sec | -15.0% | -51.9% |
| bs=10 sw=10 sl=64 | MB/s | 0.221 MB/s | 0.261 MB/s | 0.461 MB/s | -15.3% | -52.0% |
| bs=10 sw=10 sl=64 | p50 | 26,792 us | 23,372 us | 12,816 us | +14.6% | +109.1% |
| bs=10 sw=10 sl=64 | p95 | 38,040 us | 33,420 us | 16,594 us | +13.8% | +129.2% |
| bs=10 sw=10 sl=64 | p99 | 38,040 us | 33,420 us | 19,806 us | +13.8% | +92.1% |
| bs=100 sw=10 sl=64 | throughput | 776 tuples/sec | 822 tuples/sec | 969.38 tuples/sec | -5.6% | -19.9% |
| bs=100 sw=10 sl=64 | MB/s | 0.473 MB/s | 0.502 MB/s | 0.592 MB/s | -5.8% | -20.1% |
| bs=100 sw=10 sl=64 | p50 | 128,596 us | 121,866 us | 103,584 us | +5.5% | +24.1% |
| bs=100 sw=10 sl=64 | p95 | 161,670 us | 133,000 us | 109,097 us | +21.6% | +48.2% |
| bs=100 sw=10 sl=64 | p99 | 161,670 us | 133,000 us | 117,304 us | +21.6% | +37.8% |
| bs=1000 sw=10 sl=64 | throughput | 904 tuples/sec | 915 tuples/sec | 1,004 tuples/sec | -1.2% | -9.9% |
| bs=1000 sw=10 sl=64 | MB/s | 0.552 MB/s | 0.559 MB/s | 0.613 MB/s | -1.3% | -9.9% |
| bs=1000 sw=10 sl=64 | p50 | 1,105,721 us | 1,092,994 us | 1,002,357 us | +1.2% | +10.3% |
| bs=1000 sw=10 sl=64 | p95 | 1,196,679 us | 1,169,952 us | 1,046,463 us | +2.3% | +14.4% |
| bs=1000 sw=10 sl=64 | p99 | 1,196,679 us | 1,169,952 us | 1,073,661 us | +2.3% | +11.5% |
</details>
<details><summary>Raw CSV</summary>
```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,551.27,200,128000,363,0.221,26792.21,38039.97,38039.97
1,100,10,64,20,2578.96,2000,1280000,776,0.473,128595.50,161670.25,161670.25
2,1000,10,64,20,22112.32,20000,12800000,904,0.552,1105720.77,1196678.75,1196678.75
```
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(pyamber): repair LinkedBlockingMultiQueue removal paths; add unit tests [texera]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #6903:
URL: https://github.com/apache/texera/pull/6903#discussion_r3651786462
##########
amber/src/main/python/core/util/customized_queue/linked_blocking_multi_queue.py:
##########
@@ -418,15 +421,15 @@ def add_sub_queue(self, key: K, priority: int) -> Optional[SubQueue]:
added = True
break
elif pg.priority > priority:
- new_pg = LinkedBlockingMultiQueue.PriorityGroup(priority)
+ new_pg = self.PriorityGroup(priority)
new_pg.add_queue(sub_queue)
self.priority_groups.append(new_pg)
added = True
Review Comment:
When inserting a new PriorityGroup with higher priority than an existing group (pg.priority > priority), the code appends the new group to the end of priority_groups. This breaks the intended priority ordering (smaller number = higher priority) and can cause selection to prefer lower-priority groups added earlier (e.g., adding priorities 0,2 then 1 results in [0,2,1]). Use insert at the current index instead of append.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(pyamber): repair LinkedBlockingMultiQueue removal paths; add unit tests [texera]
Posted by "mengw15 (via GitHub)" <gi...@apache.org>.
mengw15 commented on code in PR #6903:
URL: https://github.com/apache/texera/pull/6903#discussion_r3651832193
##########
amber/src/main/python/core/util/customized_queue/linked_blocking_multi_queue.py:
##########
@@ -418,15 +421,15 @@ def add_sub_queue(self, key: K, priority: int) -> Optional[SubQueue]:
added = True
break
elif pg.priority > priority:
- new_pg = LinkedBlockingMultiQueue.PriorityGroup(priority)
+ new_pg = self.PriorityGroup(priority)
new_pg.add_queue(sub_queue)
self.priority_groups.append(new_pg)
added = True
Review Comment:
Confirmed — this is a real defect, not introduced here: the `i` counter in that loop is maintained for exactly this insert and is otherwise unused. `priority_groups` must stay sorted because `DefaultSubQueueSelection` walks it in order without comparing priorities, so registering a data channel before its control channel can cost control its priority. It's out of scope for this PR (which repairs the removal paths), so I filed it as #6905 and fixed it separately in #6906.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(pyamber): repair LinkedBlockingMultiQueue removal paths; add unit tests [texera]
Posted by "mengw15 (via GitHub)" <gi...@apache.org>.
mengw15 merged PR #6903:
URL: https://github.com/apache/texera/pull/6903
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(pyamber): repair LinkedBlockingMultiQueue removal paths; add unit tests [texera]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #6903:
URL: https://github.com/apache/texera/pull/6903#issuecomment-5082087512
## [Codecov](https://app.codecov.io/gh/apache/texera/pull/6903?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 78.57%. Comparing base ([`4af3a85`](https://app.codecov.io/gh/apache/texera/commit/4af3a85bcd4ecd3c1f8b5588ed4a1e25361f7ef7?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`51751fe`](https://app.codecov.io/gh/apache/texera/commit/51751feb8b38b1ee5ba041a14a6f41d14cf7ab2f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #6903 +/- ##
============================================
+ Coverage 78.39% 78.57% +0.18%
Complexity 3723 3723
============================================
Files 1161 1161
Lines 46066 46067 +1
Branches 5107 5107
============================================
+ Hits 36112 36199 +87
+ Misses 8343 8257 -86
Partials 1611 1611
```
| [Flag](https://app.codecov.io/gh/apache/texera/pull/6903/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | | *Carryforward flag |
|---|---|---|---|
| [access-control-service](https://app.codecov.io/gh/apache/texera/pull/6903/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `70.00% <ø> (ø)` | | Carriedforward from [4af3a85](https://app.codecov.io/gh/apache/texera/commit/4af3a85bcd4ecd3c1f8b5588ed4a1e25361f7ef7?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [agent-service](https://app.codecov.io/gh/apache/texera/pull/6903/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `76.76% <ø> (ø)` | | Carriedforward from [4af3a85](https://app.codecov.io/gh/apache/texera/commit/4af3a85bcd4ecd3c1f8b5588ed4a1e25361f7ef7?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [amber](https://app.codecov.io/gh/apache/texera/pull/6903/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `71.51% <ø> (ø)` | | Carriedforward from [4af3a85](https://app.codecov.io/gh/apache/texera/commit/4af3a85bcd4ecd3c1f8b5588ed4a1e25361f7ef7?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [computing-unit-managing-service](https://app.codecov.io/gh/apache/texera/pull/6903/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `20.49% <ø> (ø)` | | Carriedforward from [4af3a85](https://app.codecov.io/gh/apache/texera/commit/4af3a85bcd4ecd3c1f8b5588ed4a1e25361f7ef7?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [config-service](https://app.codecov.io/gh/apache/texera/pull/6903/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `66.66% <ø> (ø)` | | Carriedforward from [4af3a85](https://app.codecov.io/gh/apache/texera/commit/4af3a85bcd4ecd3c1f8b5588ed4a1e25361f7ef7?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [file-service](https://app.codecov.io/gh/apache/texera/pull/6903/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `67.21% <ø> (ø)` | | Carriedforward from [4af3a85](https://app.codecov.io/gh/apache/texera/commit/4af3a85bcd4ecd3c1f8b5588ed4a1e25361f7ef7?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [frontend](https://app.codecov.io/gh/apache/texera/pull/6903/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `82.64% <ø> (ø)` | | Carriedforward from [4af3a85](https://app.codecov.io/gh/apache/texera/commit/4af3a85bcd4ecd3c1f8b5588ed4a1e25361f7ef7?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [notebook-migration-service](https://app.codecov.io/gh/apache/texera/pull/6903/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `78.94% <ø> (ø)` | | Carriedforward from [4af3a85](https://app.codecov.io/gh/apache/texera/commit/4af3a85bcd4ecd3c1f8b5588ed4a1e25361f7ef7?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [pyamber](https://app.codecov.io/gh/apache/texera/pull/6903/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `95.40% <100.00%> (+1.91%)` | :arrow_up: | |
| [workflow-compiling-service](https://app.codecov.io/gh/apache/texera/pull/6903/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.14% <ø> (ø)` | | Carriedforward from [4af3a85](https://app.codecov.io/gh/apache/texera/commit/4af3a85bcd4ecd3c1f8b5588ed4a1e25361f7ef7?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
*This pull request uses carry forward flags. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/texera/pull/6903?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(pyamber): repair LinkedBlockingMultiQueue removal paths; add unit tests [texera]
Posted by "chenlica (via GitHub)" <gi...@apache.org>.
chenlica commented on PR #6903:
URL: https://github.com/apache/texera/pull/6903#issuecomment-5085730328
@mengw15 I found the description very hard to read. Can you improve it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org