Conversation
denik
force-pushed
the
denik/recreate-ensure-deleted
branch
from
September 18, 2026 09:51
eb4ce09 to
78c1a09
Compare
Collaborator
Integration test reportCommit: cf3f2c1
Top 6 slowest tests (at least 2 minutes):
|
denik
added a commit
that referenced
this pull request
Sep 18, 2026
Co-authored-by: Isaac <no-reply@databricks.com>
denik
force-pushed
the
denik/recreate-ensure-deleted
branch
from
September 18, 2026 14:02
78c1a09 to
5f3e228
Compare
denik
changed the title
A synced table's delete drops its destination Postgres table asynchronously, on a
slower schedule than the synced-table record that WaitAfterDelete polls. So even
after the delete-wait completes, the re-create can fail with 409 ALREADY_EXISTS
("Destination table ... already exists"), as seen on an AWS nightly (#6728 waited
on the wrong object). Recreate now keeps WaitAfterDelete as the fast path and, if
the create still fails with ALREADY_EXISTS, retries it until the teardown finishes
(bounded at 5m) before surfacing the result. A create that fails for any other
reason, or a resource that never re-creates the same id (e.g. job_runs), is
returned immediately.
The testserver models the race under eventual consistency: a deleted synced table
disappears from GET at once but the next create for that id conflicts once. The
recreate_capped test becomes recreate_conflict_retry, asserting the retry succeeds.
Co-authored-by: Isaac <no-reply@databricks.com>
denik
force-pushed
the
denik/recreate-ensure-deleted
branch
from
September 21, 2026 08:32
5f3e228 to
888bbc8
Compare
denik
changed the title
…from its test No measured teardown latency to size the create-retry cap, so raise it 5m->15m to match deleteIndexTimeout (the closest async-teardown analog). Convert recreate_conflict_retry off the databricks.yml.tmpl + envsubst pattern to a committed databricks.yml edited in place with update_file.py. Co-authored-by: Isaac <no-reply@databricks.com>
shreyas-goenka
approved these changes
Sep 21, 2026
| case createErr == nil: | ||
| return &struct{}{}, nil | ||
| case errors.Is(createErr, apierr.ErrAlreadyExists): | ||
| log.Warnf(ctx, "Create hit ALREADY_EXISTS; the previous delete is likely still finishing, retrying: %s", createErr) |
Contributor
There was a problem hiding this comment.
Doesn't it become an issue when, let's say, I want to rename the app I have (which should trigger a recreation), but I use the name that already exists? Won't it be stuck here for 15 minutes unnecessarily?
Shall we move this logic to a specific resource instead?
Contributor
Author
There was a problem hiding this comment.
Good question. Rather than making it per-resource setting, how about we trace what fields changed? If it's id fields, then this is a real conflict that we must abort on; if it's other immutable fields but id stays the same then we apply this retries.
Contributor
Author
There was a problem hiding this comment.
So it does not actually gets stuck because apps return RESOURCE_ALREADY_EXISTS and not ALREADY_EXISTS. However, I'd rather not rely on that, this looks accidental and other resources can use codes differently.
The recreate create-retry waits out a slow async teardown, but only makes sense when the create re-creates the same id it just deleted. If the recreate changed a provided-id field (e.g. renamed an app, or pointed a synced table at a new synced_table_id), the create targets a different id that a separate, pre-existing resource owns — no wait can free that name, so surface the ALREADY_EXISTS immediately instead of retrying for minutes. Adds local+cloud coverage for a synced-table rename onto an existing id, plus local fail-fast coverage for apps and vector search endpoints. Co-authored-by: Isaac <no-reply@databricks.com>
…edundant VS test On cloud a synced-table create with an already-taken id replaces the existing table (replace_existing) instead of returning ALREADY_EXISTS, so the rename never conflicts there and the cloud run "succeeded" unexpectedly. The retry the same-id guard gates only matches error_code ALREADY_EXISTS (postgres synced tables); apps and vector search endpoints return RESOURCE_ALREADY_EXISTS, a sibling sentinel the retry never caught, so they already fail fast. The guard is therefore only observable against the testserver — keep the synced-table test as its local regression, and drop the vector-search test, which duplicated the existing recreate/create-fails coverage. Co-authored-by: Isaac <no-reply@databricks.com>
Match the whole conflict class (ErrResourceConflict) instead of just error_code ALREADY_EXISTS: the lingering-teardown race the retry waits out can surface as either ALREADY_EXISTS (postgres synced tables) or RESOURCE_ALREADY_EXISTS (apps, vector search endpoints), and which code a backend returns should not decide whether we retry. The same-id guard still fails a rename (a provided-id change) fast, so a genuine name collision never spins for minutes regardless of code. The testserver no longer returns ALREADY_EXISTS when creating a synced table whose id already exists — the real backend replaces it rather than conflicting, so the previous rename-conflict test could not reproduce on cloud. Drop that test; the guard's regression coverage is the apps rename and the existing vector_search_endpoints/recreate/create-fails, which now exercise the retry path and would spin without the guard. The destination-table teardown race stays. Co-authored-by: Isaac <no-reply@databricks.com>
denik
changed the title
The same-id guard should attempt the rename's create exactly once. Record the requests and print the recreate's /apps calls so the golden shows a single POST /api/2.0/apps (create app-beta) after the delete — a retry regression would show it repeated. Co-authored-by: Isaac <no-reply@databricks.com>
This branch has not been deployed
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
4 participants
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.
Changes
On recreate, retry the create while it keeps hitting a resource conflict — unless the recreate changed the resource's id.
Why
The create can race the previous delete's async teardown (e.g. a synced table's destination Postgres table is dropped on a slower schedule), so it still 409s after
WaitAfterDeletereturns; retrying rides it out. A recreate that changed a provided-id field (a rename) fails fast instead, since a separate resource owns that name and no wait frees it.This pull request and its description were written by Isaac.