python: surface commit/ingest progress in wait loop warnings - #6780
Conversation
wait_for_completion and wait_end_of_input logged only elapsed time while blocked, giving no way to tell a slow pipeline from a stalled one (e.g. the adaptive-join balancer barrier) without pulling server-side stats separately. Both now reuse the stats already fetched each poll to report commit progress or processed/input record counts, and reuse LongOperationWarning for wait_end_of_input instead of logging nothing. Also fixes GlobalPipelineMetrics.from_dict, which never converted the server's commit_progress payload into a CommitProgressSummary -- it stayed a raw dict via the blind __dict__.update(d), so the class existed but nothing could actually use it. The immediate reason for this is the CI failure in #6709. It's unclear whether it's hanging or just slow. This should help. Signed-off-by: Ben Pfaff <blp@feldera.com>
mythical-fred
left a comment
There was a problem hiding this comment.
Wait loops now say what they're actually waiting on — commit_progress while a transaction is committing, processed/input records otherwise — instead of just elapsed seconds. That is the difference between "slow" and "stalled" in a CI hang, so this is the right shape of fix for the underlying #6709 symptom.
Also finishes plumbing CommitProgressSummary into GlobalPipelineMetrics.from_dict — __dict__.update(d) had left it as a raw dict, so callers of .commit_progress were seeing an untyped payload rather than the class. Straightforward correctness fix.
wait_end_of_input now uses LongOperationWarning too, matching wait_for_completion in both semantics and cadence. Good consolidation.
Minor mechanical notes, non-blocking:
- The
latest_stats.global_metrics.progress_summary()lambdas rely onlatest_statsbeing assigned before anylong_op.check(). Both call sites are structured so.stats()runs first inside the loop before.check(), so this is safe as written — worth a one-line comment if anyone ever moves the ordering. CommitProgressSummary.__str__mirrors the RustDisplayimpl, including the "1 operators" pluralization quirk. Fine to keep for symmetry, but flagging so nobody "fixes" it in isolation later.CommitProgressSummary.from_dictstill uses__dict__.update(d)— same blind-update pattern that produced the bug being fixed here forGlobalPipelineMetrics. Not this PR's job to convert them all, but worth an issue to enumerate the remaining blind-update sites instats.py.
Signed-off, no AI trailers, unit test coverage for both parsing paths and both progress-summary branches.
APPROVE.
| return status | ||
|
|
||
| def __str__(self) -> str: | ||
| # Mirrors CommitProgressSummary's Display impl in |
| # Reused by the warning message below so a stalled ingest can be told | ||
| # apart from a slow one; check_end_of_input() would fetch and discard it. | ||
| latest_stats = None | ||
| long_op = LongOperationWarning( |
There was a problem hiding this comment.
I would perhaps name these based on what they are waiting for rather than the class. wait_for_input_end.
There was a problem hiding this comment.
That's reasonable. I added a commit that renames all of these variables.
This is a little clearer. Suggested by Mihai. Signed-off-by: Ben Pfaff <blp@feldera.com>
wait_for_completion and wait_end_of_input logged only elapsed time while blocked, giving no way to tell a slow pipeline from a stalled one (e.g. the adaptive-join balancer barrier) without pulling server-side stats separately. Both now reuse the stats already fetched each poll to report commit progress or processed/input record counts, and reuse LongOperationWarning for wait_end_of_input instead of logging nothing.
Also fixes GlobalPipelineMetrics.from_dict, which never converted the server's commit_progress payload into a CommitProgressSummary -- it stayed a raw dict via the blind dict.update(d), so the class existed but nothing could actually use it.
The immediate reason for this is the CI failure in #6709. It's unclear whether it's hanging or just slow. This should help.