Wait for the rows when re-executing an expired result - #50
Merged
Conversation
Exercising execute-query against a live workspace corrected a wire assumption and, with it, a real defect. execute-query only starts the re-execution. It answers HTTP 200 with state PENDING and no manifest, and the rows appear on the ordinary query-result endpoint a moment later. The client returned that first acknowledgement, so ReExecuteQueryAsync produced null and `export last` told the user to ask the question again -- while the warehouse query they had just paid for completed and was discarded. It now polls for the rows, using the same backoff and timeout as every other wait in this client. The contract test covering this path stubbed a completed response, which Databricks does not return, so the test agreed with the bug. That is the general failure worth naming: a stub cannot catch an error in the stub. The new contract test reproduces the real PENDING acknowledgement, and fails without the fix. Alongside it is a live test, because this is the second time in this cycle that a fixture encoded a shape the service does not actually produce. The live suite is 9 tests, all passing. Still unreached is the QUERY_RESULT_EXPIRED state that triggers recovery: Databricks expires the cache on its own schedule, hours later, and there is no way to force it. So the recovery is verified and the condition it recovers from is simulated -- which is what the docs now say rather than implying both.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closing the last live-coverage gap I could reach found a real defect — and the test that was
supposed to cover it had been agreeing with the bug.
What the live workspace says
POST …/attachments/{id}/execute-querystate: PENDING, no manifest, no rowsGET …/attachments/{id}/query-resultmoments laterstate: SUCCEEDED, rows presentexecute-queryonly starts the re-execution.The defect
ReExecuteQueryAsyncreturned that first acknowledgement.FetchQueryResultAsyncreturnsnullwhen there is no manifest, so the documented
QUERY_RESULT_EXPIREDrecovery produced nothing —and
ExportCommandthen threw:The user is told to start over while the warehouse query they just paid for completes successfully
a second later and is discarded. It now polls for the rows, with the same backoff and timeout as
every other wait in this client.
Why the existing test did not catch it
The contract test stubbed a completed response — a shape Databricks does not return. The test
encoded the same wrong assumption as the code, so it passed.
A stub cannot catch an error in the stub. That is the second time in this cycle a fixture
encoded a shape the service does not produce, so the replacement is both:
PENDINGacknowledgement, which fails without the fixA_re_executed_query_returns_its_rows, run against the real workspaceWhat is still not reached, and why
The
QUERY_RESULT_EXPIREDstate that triggers recovery. Databricks expires the cache on its ownschedule, hours later, and there is no way to force it. So the recovery is verified and the
condition it recovers from is simulated — which is what
compatibility.mdandROADMAP.mdnowsay, rather than implying both are covered.
Verification
230 offline tests and 9 live tests green — the live suite grew from 8. Build clean under
warnings-as-errors; format clean.