Yicong-Huang commented on code in PR #6906:
URL: https://github.com/apache/texera/pull/6906#discussion_r3738766123
##########
amber/src/main/python/core/util/customized_queue/linked_blocking_multi_queue.py:
##########
@@ -421,9 +421,12 @@ def add_sub_queue(self, key: K, priority: int) ->
Optional[SubQueue]:
added = True
break
elif pg.priority > priority:
+ # pg has a lower priority than the new group, so the
new
+ # group belongs in front of it: priority_groups must
stay
+ # sorted because the selection strategy walks it in
order.
new_pg = self.PriorityGroup(priority)
new_pg.add_queue(sub_queue)
- self.priority_groups.append(new_pg)
+ self.priority_groups.insert(i, new_pg)
Review Comment:
Same class of port divergence three lines up. Upstream lbmq 0.6.0 does
`subQueues.putIfAbsent(key, subQueue)`; `:413-414` overwrites unconditionally.
A repeated key then installs a SubQueue with `priority_group=None`, in no
group. A later `put` bumps `total_count`, `get()` wakes, `get_next()` returns
`None`, and `:318` raises `AttributeError: 'NoneType' object has no attribute
'dequeue'` under `take_lock`. The docstring at `:401` describes the Java
behaviour, not this one.
`_queue_ids` (`internal_queue.py:79`) makes it unreachable today, so not a
blocker — a follow-up issue is fine.
##########
amber/src/test/python/core/models/test_internal_queue.py:
##########
@@ -116,13 +116,6 @@ def test_it_accepts_all_recognized_element_types(
assert queue.is_empty()
Review Comment:
The NOTE at lines 105-107 is now false. And it is an instruction, not just a
description: it tells the reader to keep registering control before data "to
preserve control-priority semantics" — the constraint this PR removes. It and
the `xfail` you dropped are twins from the same commit (#6444, `c8826255d`).
Please delete the three NOTE lines.
##########
amber/src/test/python/core/models/test_internal_queue.py:
##########
@@ -116,13 +116,6 @@ def test_it_accepts_all_recognized_element_types(
assert queue.is_empty()
@pytest.mark.timeout(2)
- @pytest.mark.xfail(
- reason=(
- "LinkedBlockingMultiQueue.add_sub_queue does not currently insert
new "
- "priority groups ahead of lower-priority ones, so registering data
before "
- "control can break control-priority ordering."
- )
- )
def
test_control_elements_dequeue_before_data_even_if_data_channel_registered_first(
Review Comment:
This test is now the only guard, and it sits one layer above the fix. The
class's own suite, `test_linked_blocking_multi_queue.py`, already asserts on
`priority_groups` (`:455-456`). But every `add_sub_queue` call in it registers
in ascending or equal priority order, so none reaches the `insert` branch.
Adding one that registers out of order and asserts the group priorities come
out sorted would pin the invariant where it lives. That is your 24-permutation
check, in the repo.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
