villebro commented on code in PR #43782:
URL: https://github.com/apache/superset/pull/43782#discussion_r3915337758
##########
UPDATING.md:
##########
@@ -84,9 +84,12 @@ completion transport for async chart-data — see the note on
the interval poll)
> When it is **enabled**, completion is delivered over the socket and the
> recurring interval poll does not run; a one-shot `status_changes` catch-up on
> waiter registration and on socket reconnect reconciles anything missed while
-> disconnected. Redis Pub/Sub is lossy, so a `task.status` lost while the
socket
-> stays open is only recovered on the next reconnect/registration or, failing
-> that, a per-request give-up timeout (a page reload re-establishes state).
+> disconnected. The socket accelerates delivery over the authoritative
+> `status_changes` API rather than replacing it: Redis Pub/Sub is best-effort
+> (at-most-once, no replay), so a disconnect is reconciled by the catch-up on
+> reconnect/registration; in the rare case a `task.status` is missed while the
+> socket stays open, completion still resolves on the next
reconnect/registration
+> or the per-request give-up (and a page reload re-establishes state).
Review Comment:
Good catch — reworded in ad8e95f9d7. You're right that a plain give-up
rejects rather than resolves. The nuance is the one-shot catch-up added in
3eaf2dcac6: before the give-up times out it now runs a final `status_changes`
read, so a chart whose query *actually finished* resolves, and only an
unconfirmed one ends in a **bounded error** (reload re-establishes state). The
note now says exactly that instead of implying completion always resolves.
##########
superset/tasks/decorators.py:
##########
@@ -251,8 +251,9 @@ def _merge_options(self, override_options: TaskOptions |
None) -> TaskOptions:
"""
Merge decorator defaults with call-time overrides.
- Call-time options take precedence over decorator defaults.
- For timeout, an explicit None in TaskOptions disables the decorator
timeout.
+ Call-time options take precedence over decorator defaults. A call-time
+ ``timeout`` overrides only when set to a concrete value; ``None``
inherits
+ the decorator's timeout.
Review Comment:
Fixed in ad8e95f9d7. The inline merge comment now matches the docstring and
the implementation: a call-time `timeout` is used only when it's a concrete
value; `None` falls back to the decorator timeout (it does not disable it) — so
no opposite guidance at the decision point.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
