Yicong-Huang commented on code in PR #8276:
URL: https://github.com/apache/texera/pull/8276#discussion_r3910929114
##########
amber/src/main/python/core/architecture/packaging/output_manager.py:
##########
@@ -169,7 +169,9 @@ def start_writer(uri: str, name_prefix: str, registry:
dict) -> None:
)
def get_port(self, port_id=None) -> WorkerPort:
- return list(self._ports.values())[0]
+ if port_id is None:
+ return next(iter(self._ports.values()))
Review Comment:
The empty-`_ports` failure mode changes shape here.
`list(self._ports.values())[0]` raised `IndexError: list index out of range`.
`next(iter(...))` raises a bare, message-less `StopIteration` — and through the
emit generator expressions, PEP 479 turns that into `RuntimeError: generator
raised StopIteration`.
Empty `_ports` is a documented legitimate state — `is_missing_output_ports`
exists for it — so the branch is not dead by construction. Advisory rather than
blocking because no production caller reaches `get_port()` in that state.
The narrow fix is to keep the old failure visible to callers — wrap the
`next(...)` so an exhausted iterator surfaces as the `IndexError` this function
used to raise, rather than as a bare `StopIteration` the generator machinery
reinterprets.
##########
amber/src/test/python/core/architecture/packaging/test_output_manager.py:
##########
@@ -666,6 +666,18 @@ def
test_get_port_ids_returns_added_ports_in_insertion_order(self, output_manage
output_manager.add_output_port(port_b, MagicMock())
assert output_manager.get_port_ids() == [port_a, port_b]
+ def test_get_port_honors_the_requested_port_id(self, output_manager):
+ port_a = PortIdentity(id=0, internal=False)
+ port_b = PortIdentity(id=1, internal=False)
+ output_manager.add_output_port(port_a, MagicMock(name="port_a"))
+ output_manager.add_output_port(port_b, MagicMock(name="port_b"))
+ assert output_manager.get_port(port_b) is output_manager._ports[port_b]
Review Comment:
This asserts against the private `_ports` dict, so it passes for any
implementation that stores the port under that key — including one that returns
the wrong schema. The file's own pattern at :397 asserts on the observable
schema instead, which is what the caller actually depends on.
##########
amber/src/main/python/core/architecture/packaging/output_manager.py:
##########
@@ -169,7 +169,9 @@ def start_writer(uri: str, name_prefix: str, registry:
dict) -> None:
)
def get_port(self, port_id=None) -> WorkerPort:
- return list(self._ports.values())[0]
+ if port_id is None:
+ return next(iter(self._ports.values()))
Review Comment:
`port_id` now carries a two-mode contract — an explicit `PortIdentity`, or
the implicit single-port fallback — but it is neither annotated nor documented.
`InputManager.get_port` and `add_output_port` in this same file both annotate
theirs.
--
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]
