github-actions[bot] commented on code in PR #41174:
URL: https://github.com/apache/beam/pull/41174#discussion_r4540816819
##########
sdks/python/apache_beam/runners/worker/opcounters.py:
##########
@@ -346,6 +346,7 @@ def should_sample(self):
def restart_sampling(self):
self._sample_counter = 0
+ self._next_sample = 0
Review Comment:
**🟢 Low**: Resetting `self._next_sample = 0` here also fixes a subtle bug
where calling `restart_sampling()` after `self._sample_counter > 30` previously
left `_next_sample` at its old value and skipped sampling the initial elements
after a restart. Consider adding a unit test in `opcounters_test.py` that
advances the counter past 30 elements, calls `restart_sampling()`, and verifies
`should_sample()` returns `True` for the next 10 elements.
##########
sdks/python/apache_beam/runners/worker/sdk_worker.py:
##########
@@ -117,6 +117,9 @@ def get_short_id(self, monitoring_info):
not assigned already.
"""
key = monitoring_infos.to_key(monitoring_info)
+ short_id = self._info_key_to_short_id.get(key)
Review Comment:
**🟡 Medium**: With this lock-free fast path, another thread can observe
`short_id` in `self._info_key_to_short_id` as soon as line 138
(`self._info_key_to_short_id[key] = shortId`) executes inside the `with
self._lock:` block, before line 139 (`self._short_id_to_info[shortId] =
payload_cleared`) runs. If a context switch happens between lines 138 and 139,
a concurrent caller can receive `shortId` and pass it to `get_infos()`, raising
a `KeyError` on `self._short_id_to_info[short_id]`.
To make the lock-free read safe, populate `self._short_id_to_info[shortId] =
payload_cleared` **before** publishing `self._info_key_to_short_id[key] =
shortId` at lines 138–139.
--
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]
