Yicong-Huang commented on code in PR #8282:
URL: https://github.com/apache/texera/pull/8282#discussion_r4059004325
##########
amber/src/main/python/core/util/buffer/timed_buffer.py:
##########
@@ -36,7 +36,7 @@ def get(self, flush: bool = False) ->
Iterator[ConsoleMessage]:
if (
flush
or len(self._buffer) >= self._max_message_num
- or (datetime.now() - self._last_output_time).seconds
+ or (datetime.now() - self._last_output_time).total_seconds()
Review Comment:
**Advisory:**
With the old `.seconds`, a backward clock step normalised to a large
positive value. A naive-local DST fall-back or an NTP step therefore fired this
branch once, reset `_last_output_time` at `:42`, and healed itself. With
`total_seconds()` the same step yields a negative float that never crosses the
threshold, so the time-based flush stays suppressed for the whole jump. I
checked the arithmetic: `timedelta(seconds=-3600).seconds` is `82800`,
`.total_seconds()` is `-3600.0`. The in-repo answer is
`QueryWorkerStatisticsHandler.scala:96`, which measures this kind of window
with `System.nanoTime()`. The Python equivalent is `time.monotonic()`. Advisory
because no production caller reaches this branch today.
##########
amber/src/test/python/core/util/buffer/test_timed_buffer.py:
##########
@@ -129,9 +129,15 @@ def test_exceeding_max_message_num_triggers_flush(self,
clock):
class TestFlushOnTime:
+ def test_subsecond_interval_triggers_flush(self, clock):
Review Comment:
**Advisory:**
This pins the sub-second comparison in the flushing direction only. The
suite already asserts "does not flush" at 1 s against 2000 ms (`:147`) and at 0
s (`:168`), both whole-second cases. Nothing asserts that 0.4 s against a 500
ms interval still withholds, which is the case that would catch a rounding-up
implementation as surely as this one catches rounding down. Six lines beside
the test you already added.
##########
amber/src/main/python/core/util/buffer/timed_buffer.py:
##########
@@ -36,7 +36,7 @@ def get(self, flush: bool = False) ->
Iterator[ConsoleMessage]:
if (
flush
or len(self._buffer) >= self._max_message_num
- or (datetime.now() - self._last_output_time).seconds
+ or (datetime.now() - self._last_output_time).total_seconds()
>= self._max_flush_interval_in_ms / 1000
Review Comment:
**Polish:**
`test_console_message_manager.py:57` still quotes `(now -
_last_output_time).seconds >= 1`, the expression this comparison replaces, and
the 1-second margin that comment justifies is now 500 ms. The test itself is
fine, since it re-stamps `_last_output_time` on the line before the assertion.
A grep confirms this is the only surviving `timedelta.seconds` reference in the
tree.
##########
amber/src/main/python/core/util/buffer/timed_buffer.py:
##########
@@ -36,7 +36,7 @@ def get(self, flush: bool = False) ->
Iterator[ConsoleMessage]:
if (
flush
Review Comment:
**Advisory:**
A question rather than a claim. The linked issue states a user-visible
symptom, that the default 500 ms console flush waits for a full second, and I
could not find a path that produces it. All five call sites of
`MainLoop._check_and_report_console_messages` pass `force_flush=True`
(`main_loop.py:270`, `:301`, `:849`, `:854`, `:871`). `flush` short-circuits
this comparison, so the branch this PR fixes is not reached in the running
worker. Is there an intended caller that stops forcing? The fix is correct and
worth merging either way.
--
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]
