Re: [PR] fix(pyamber): keep LinkedBlockingMultiQueue priority groups sorted [texera]

["Yicong-Huang (via GitHub)" <gi...@apache.org>]

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: notifications-unsubscribe@texera.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org