Uh oh!
There was an error while loading. Please reload this page.
Add deferrable mode to Databricks SQL warehouse operators - #71752
Conversation
@eladkal@moomindani ni this is the #70088 deferrable follow-up, could you review when you have a chance? |
moomindani
left a comment
There was a problem hiding this comment.
Thanks — the Phase 1 contract is preserved carefully here, and the state-machine edge cases we went through on #70088 (stale STOPPED not terminal, deletion mid-wait, the deadline boundary) are all covered by deterministic tests rather than sleeps.
I validated the deferrable paths against a real workspace: Airflow 3.2.2, this branch's provider installed from source, a dedicated throwaway serverless warehouse (2X-Small), triggerer doing the waiting.
| scenario | task state | evidence |
|---|---|---|
deferrable start from STOPPED | deferred -> success | trigger logged is STARTING; waiting for RUNNING, warehouse reached RUNNING |
| deferrable stop | deferred -> success | warehouse reached STOPPED |
start against an already-RUNNING warehouse | success, never deferred | no DeferTask for the task |
wait_for_termination=False stop | success, never deferred | transition still requested, warehouse stopped afterwards |
timeout=1 start | deferred -> failed | DatabricksWarehouseError: ... did not reach RUNNING within 1s; last state: STARTING. |
So the behavioural claims in the description hold up where I could exercise them. Unit tests reproduce your numbers as well (43 + 14 + 13, and the full provider suite at 922 passed / 12 skipped).
Two things I would like settled before merge, both left inline:
- The changelog entry is inserted under the
7.18.1header, which is an already-released version, so a shipped release would advertise a feature it does not contain. Perproviders/AGENTS.mdand theNOTE TO CONTRIBUTORSblock at the top of that file, routine feature entries are collected by the release manager from commit messages anyway — the five lines can just go. - The deferrable
timeoutis a duration recomputed inside everyrun(), so it restarts from zero on each triggerer restart or HA rebalance, while the synchronous path fails deterministically.DatabricksSQLStatementExecutionTriggernext door serializes an absoluteend_timefor exactly this reason. Either follow that pattern or state the per-run semantics explicitly — right now the docstring and the PR description claim the opposite of what the code does.
The rest is nits, also inline. One out-of-scope note worth being aware of rather than fixing here: these operators' first deferrable surface inherits #71525, so on core 3.0.x the trigger cannot fetch the connection at all (3.1.0+ is fine). Whether that caveat belongs in this page or centrally is a call for the docs discussion happening on #71667.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Thanks for the live workspace check @moomindani i responded to comments and commited the latest change The follow-up is on the branch. Changelog Features under 7.18.1 is gone. The trigger now serializes an absolute end_time the same way DatabricksSQLStatementExecutionTrigger does. a_get_warehouse shares the GET path with get_warehouse. The on_kill override and the unpinned test are gone. The typed state JSON stays for sibling trigger symmetry, and the description now mentions the Phase 1 to_json restore. The 3.0.x connection fetch caveat from 71525 is still out of scope here. |
moomindani
left a comment
There was a problem hiding this comment.
Thanks — all five points are addressed, and I re-verified on 38bc535d4a rather than taking it on trust.
The changelog is back to the released 7.18.1 section untouched. The deadline is now an absolute end_time computed by the operator, serialized, and compared with time.time() in run(), matching DatabricksSQLStatementExecutionTrigger — and the two new tests pin it where it matters: the round trip preserves end_time, and a trigger resumed with an already-passed end_time times out immediately instead of opening a fresh window. That was the part I cared about most. on_kill and its vacuous test are gone with the rationale kept in the class docstring, and a_get_warehouse_state now goes through a_get_warehouse; the remaining duplicated path suffix matches how start_warehouse / stop_warehouse declare theirs in that file, so no objection from me. Keeping the typed state payload for sibling symmetry with the reasoning recorded is a fine call.
Real workspace re-run on this head (Airflow 3.2.2, provider installed from the branch, dedicated throwaway serverless warehouse): deferrable start deferred -> success, deferrable stop deferred -> success, and timeout=1deferred -> failed with did not reach RUNNING within 1s. 203 unit tests pass across the three files.
One observation from that last run, not a request: with a timeout shorter than the defer round-trip, the absolute deadline has already passed by the time the triggerer picks the trigger up, so it never polls and the message reports last state: unknown where the synchronous path would have polled once and named a state. That is the correct consequence of honouring an absolute deadline and is invisible at realistic timeouts — just noting that the boundary now differs slightly between the two paths.
LGTM.
Drafted-by: Claude Code (Opus 5); reviewed by @moomindani before posting
Vamsi-klu
commented
Aug 19, 2026
@eladkal as @moomindani approved the PR, Can i get your thoughts on this please? |
eladkal
commented
Aug 19, 2026
can you please update the comment in #21377 (comment) with checklist to the actual steps in the plan and which PR solved which step? That way we can easily track what is left open |
Start and stop waits from apache#70088 held a worker for the whole warehouse lifecycle. This follow-up lands the agreed additive triggerer path so those waits no longer occupy a worker slot. Co-authored-by: Cursor <cursoragent@cursor.com>
A duration recomputed inside the trigger started a fresh wait after every triggerer restart or HA rebalance, so a warehouse that never reached its target could stay deferred well past the configured timeout.
Sphinx treats the HA term as a misspelling and fails the provider docs job. Co-authored-by: Cursor <cursoragent@cursor.com>
16dfb9e to
8f7d2d3CompareHi @eladkal thanks a ton for the comment Updated the tracking comment: #21377 (comment) #70088 is Phase 1. This PR is the deferrable follow-up. Create/delete, name lookup, edit, and a live system-test Dag are still open. |
Uh oh!
There was an error while loading. Please reload this page.
This adds an additive deferrable path for the Databricks SQL warehouse start and stop operators, so lifecycle waits can run on the triggerer instead of holding a worker.
related: #70088
related: #21377
Problem
Airflow's Databricks provider can start and stop an existing SQL warehouse, but Phase 1 waits hold a worker for the entire poll. Dag authors who want triggerer-based waiting currently have no
deferrablepath on these operators.What changed
DatabricksHook.a_get_warehouse_stateas the async GET mirror ofget_warehouse_state.WarehouseState.to_json/from_jsonfor trigger event round-trip.DatabricksWarehouseStateTriggernext to the provider's existing triggers, with serialize, local monotonic timeout, retry_args validation, and a documented no-opon_kill.deferrableon bothDatabricksStartWarehouseOperatorandDatabricksStopWarehouseOperatorusing thedefault_deferrablepattern.No request-body workaround or new dependency is introduced. The implementation follows the Databricks SQL Warehouses API and keeps the Phase 1 wait contract.
Scope
This is the Phase 2 deferrable follow-up agreed on #70088: async hook, serialized trigger, operator completion path, and compatibility tests. Create/delete, edit, warehouse-by-name resolution, and a live Databricks system-test Dag remain outside this PR so the follow-up stays reviewable and independently useful.
Behavior and compatibility
deferrabledefaults off and honors[operators] default_deferrable.wait_for_termination,polling_period_seconds, andtimeoutare unchanged.RUNNINGwarehouse and stopping an alreadySTOPPEDwarehouse remain no-ops and do not defer.wait_for_termination=Falsestill returns after requesting the transition and does not defer.STARTING/STOPPINGtransitions are reused instead of issuing duplicate requests, then deferred if waiting.STOPPINGcontinues on the triggerer; Databricks API transition rejections propagate unchanged.STOPPEDwhile waiting forRUNNINGis not terminal; the trigger keeps polling untilRUNNING, deletion, or timeout.time.monotonic()locally in the trigger, starts no new poll after the configured deadline, and still honors a target or deletion state returned by a poll that began before the deadline.execute_completemapssuccess/deleted/timeoutto the same messages as Phase 1.Validation
uv run --project providers/databricks pytest providers/databricks/tests/unit/databricks/operators/test_warehouse.py providers/databricks/tests/unit/databricks/hooks/test_databricks.py::TestWarehouseLifecycle providers/databricks/tests/unit/databricks/triggers/test_databricks.py::TestDatabricksWarehouseStateTrigger providers/databricks/tests/unit/databricks/triggers/test_databricks.py::test_trigger_init_rejects_non_serializable_retry_args— 70 passed.prek run --from-ref upstream/main --stage pre-commit— passed.Reviewer evidence
This PR has no UI surface, so before/after screenshots and browser validation are not applicable. The REST boundary is covered with an autospecced async GET assertion, operator deferral is covered with a specced hook, and trigger serialize/run/timeout/on_kill are covered with mocked warehouse state. No Databricks workspace credentials were used or required for these deterministic lifecycle tests.
The Phase 2 scope was posted on #70088 before this follow-up: #70088 (comment)
Was generative AI tooling used to co-author this PR?
Generated-by: Cursor Grok 4.6 following the guidelines