Skip to content

Tighten the Spark and SQL cancellation notes in the docs #920

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
laughingman7743 merged 3 commits into from
Oct 3, 2026
Merged

Tighten the Spark and SQL cancellation notes in the docs #920

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b304b3c
Describe Spark cancellation as usually taking effect
laughingman7743 Oct 3, 2026
0e66573
Say when a Spark start request is abandoned on an interrupt
laughingman7743 Oct 3, 2026
0c64829
Include a failed wait in the cancellation failure notes
laughingman7743 Oct 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/aio.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ With `kill_on_interrupt` enabled, which is the default, cancelling the task whil
requests cancellation of the query, waits until it reaches a terminal state, and then raises `asyncio.CancelledError`.
Cancellation is a best-effort request, so the query can still end as `SUCCEEDED` or `FAILED`.
The `query_id` property keeps the ID of the cancelled query.
If the cancellation request fails, `asyncio.CancelledError` is raised with the error as its cause.
If the cancellation request or that wait fails, `asyncio.CancelledError` is raised with the error as its cause.
Cancelling the task while `execute()` is still starting the query first waits for the start request to finish,
and then cancels the query it started in the same way.
If the task is cancelled before `execute()` begins the request, the request is never sent.
Expand Down
10 changes: 7 additions & 3 deletions docs/spark.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,8 +256,10 @@ The `cancel()` method sends a [StopCalculationExecution](https://docs.aws.amazon
request for the calculation. It does not terminate the session.
Athena cancels the calculation on a best-effort basis:

- A running Spark job, such as a DataFrame action, stops within seconds.
The calculation ends in the `CANCELED` state, and the session remains usable for later calculations.
- A running Spark job, such as a DataFrame action, usually stops within seconds.

@laughingman7743 laughingman7743 Oct 3, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self-review rounds 1 and 2 (docs-only): range 2fcbcdbe7ae01b31a4a3bb613e6bccdfcd382014..b304b3c5a12546f7b8a0288b60e2a05cad10e326. Result: CLEAN

Round 1, content against the code:

  • cancel() and the interrupt paths send StopCalculationExecution once, through SparkBaseCursor._cancel / AioSparkCursor._cancel, and wait for whatever terminal state Athena reports. The cursor guarantees no particular outcome, which matches "usually".
  • The new bullet does not conflict with the existing start-phase sentence further down the section ("can occasionally have no effect, so the calculation can still end in the COMPLETED state").

Round 2, claims against the evidence:

The calculation then ends in the `CANCELED` state, and the session remains usable for later calculations.
- A request sent right after the calculation starts can occasionally have no effect.
The calculation then runs as if it had not been canceled.

@laughingman7743 laughingman7743 Oct 3, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent review (relayed): Codex CLI, reported model gpt-6-astra, reasoning effort max (as reported), session 01a0ff8d-b6b0-70c2-b1ad-802d54551ff1. Result: FINDINGS (unchanged text in the same section)

Scope: 2fcbcdbe7ae01b31a4a3bb613e6bccdfcd382014..b304b3c5a12546f7b8a0288b60e2a05cad10e326 plus the whole Cancellation section. The reviewer ran in a read-only detached snapshot with no PR framing, against the measured facts supplied in the prompt. Static review. The snapshot and the PR worktree were unchanged afterwards.

Covered surfaces: the exact hunk, the entire Cancellation section, cancellation/startup/polling/state/token paths in all four named source files, and the supplied Athena observations. Static review only.

FINDINGS

  • P2 — docs/spark.md:290: Startup interruption does not always wait and expose a calculation ID. If KeyboardInterrupt arrives before the helper begins the request, future.cancel() succeeds. The interrupt propagates immediately, the helper sends no request, and calculation_id remains None, having been cleared by execute(). Qualify the wait/cancel/ID claims to requests the helper has already begun. This finding concerns unchanged text in the requested section.

The changed list matches the supplied observations and introduces no contradiction.

Disposition: fixed in the next commit. Verified: BaseCursor._start_execution() abandons a request the helper has not begun (future.cancel()), and the interrupt then propagates with calculation_id still None, because SparkCursor.execute() clears it. docs/spark.md now adds after the start-phase sentences: "If execute() has not begun the request when the interrupt is handled, the request is never sent and calculation_id is None." The wording matches docs/usage.md.

- Python code that runs on the driver without a Spark job, such as `time.sleep()`, runs to completion.
The calculation ends in the `COMPLETED` state, and the session rejects new calculations until then.
- Canceling a calculation that has already finished does not raise an error or change its state.
Expand All @@ -283,12 +285,14 @@ with conn.cursor() as cursor:
With `kill_on_interrupt` enabled, which is the default, a `KeyboardInterrupt` while `execute()` waits for the calculation
requests cancellation, waits until the calculation reaches a terminal state, and then propagates.
The `state` property returns that terminal state.
If the cancellation request fails, the `KeyboardInterrupt` propagates with the error as its cause.
If the cancellation request or that wait fails, the `KeyboardInterrupt` propagates with the error as its cause,

@laughingman7743 laughingman7743 Oct 3, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self-review of the repairs (both perspectives) and independent follow-up (relayed). Result: CLEAN

Self-review of b304b3c5..0c64829d34e36dd87a896196f44f88e267117734, checked against the code:

  • The abandon sentence matches BaseCursor._start_execution() (future.cancel()) and the reset in SparkCursor.execute().
  • The failed-wait wording matches BaseCursor._poll() / AioBaseCursor._poll(), which chain any Exception from _cancel_and_wait().
  • Spark state is None on that path because the execution is never stored.
  • just docs lint: 0 errors.

Codex CLI, reported model gpt-6-astra, reasoning effort max (as reported), session 01a0ff96-3239-7133-96c1-f5c4d1bbae53. Scope: 2fcbcdbe..0c64829d34e36dd87a896196f44f88e267117734. In one pass, the reviewer checked every sentence of the Spark Cancellation section, including its asyncio counterpart, the usage "Query cancellation on interrupt" section, and the aio "Task cancellation" subsection, against nine source files and the measured facts. Read-only snapshot, static review. The snapshot and the PR worktree were unchanged afterwards.

Covered: every sentence in Spark’s Cancellation section and asyncio counterpart, Query cancellation on interrupt, and Task cancellation, against all nine named source files and the supplied Athena observations.

Previous finding: resolved. The notes cover failed cancellation requests and failed subsequent waits, including Spark’s state=None.

CLEAN — no remaining actionable false statements found. Static review only; no files changed or tests/network operations performed.

and the `state` property returns `None`.

A `KeyboardInterrupt` while `execute()` is still starting the calculation first waits for the
[StartCalculationExecution](https://docs.aws.amazon.com/athena/latest/APIReference/API_StartCalculationExecution.html)
request to finish, and then cancels the calculation it started in the same way.
The `calculation_id` property returns that calculation's ID.
If `execute()` has not begun the request when the interrupt is handled, the request is never sent and `calculation_id` is `None`.

@laughingman7743 laughingman7743 Oct 3, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent follow-up review (relayed): Codex CLI, reported model gpt-6-astra, reasoning effort max (as reported), session 01a0ff92-5cc9-71d0-a765-4aad595cadc6. Scope: b304b3c5..0e6657343e78c47f3245df4f4d337bac0537e524 plus the whole Cancellation section, read-only snapshot, static review. The snapshot and the PR worktree were unchanged afterwards.

Covered: the new diff, the entire Cancellation section, all four named source files, and the supplied Athena observations. Static review only.

Previous finding: resolved by docs/spark.md:294.

FINDINGS

  • P2 — docs/spark.md:287: Terminal-state guarantee omits polling failures. During a long time.sleep(), Ctrl-C can successfully send the stop request, followed by status polling exhausting its retries. The calculation continues, but KeyboardInterrupt propagates and cursor.state remains None: the state assignment never completes, and the handler chains the polling error. Lines 286–288 should qualify the wait/state guarantee for polling failures as well as cancellation-request failures.

Disposition: fixed in 0c64829 (pre-existing text from #833, kept in scope because this PR tightens this section).

Verified: BaseCursor._poll() chains any Exception from _cancel_and_wait(), including a failed status request, as __cause__. On that path, SparkBaseCursor._cancel_and_wait() never stores the execution, so state returns None after the reset in SparkCursor.execute().

docs/spark.md now says: "If the cancellation request or that wait fails, the KeyboardInterrupt propagates with the error as its cause, and the state property returns None." The SQL notes in docs/usage.md and docs/aio.md (from #853) had the same omission, and now say "the cancellation request or that wait".

A second `KeyboardInterrupt` during this wait propagates at once without cancelling the calculation.
A cancellation request sent right after a calculation starts can occasionally have no effect, so the calculation can still end in the `COMPLETED` state.

Expand Down
2 changes: 1 addition & 1 deletion docs/usage.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -507,7 +507,7 @@ With `kill_on_interrupt` enabled, which is the default, a `KeyboardInterrupt` wh
requests cancellation, waits until the query reaches a terminal state, and then propagates.
Cancellation is a best-effort request, so the query can still end as `SUCCEEDED` or `FAILED`.
The `query_id` property keeps the ID of the interrupted query.
If the cancellation request fails, the `KeyboardInterrupt` propagates with the error as its cause.
If the cancellation request or that wait fails, the `KeyboardInterrupt` propagates with the error as its cause.

A `KeyboardInterrupt` while `execute()` is still starting the query first waits for the
[StartQueryExecution](https://docs.aws.amazon.com/athena/latest/APIReference/API_StartQueryExecution.html)
Expand Down
Loading