Skip to content

[fix](regression) Retry repository drop on backup lock contention#65691

Draft
shuke987 wants to merge 2 commits into
apache:masterfrom
shuke987:codex/fix-repository-auth-backup-lock-wait-master
Draft

[fix](regression) Retry repository drop on backup lock contention#65691
shuke987 wants to merge 2 commits into
apache:masterfrom
shuke987:codex/fix-repository-auth-backup-lock-wait-master

Conversation

@shuke987

@shuke987 shuke987 commented Jul 16, 2026

Copy link
Copy Markdown
Collaborator

What problem does this PR solve?

Issue Number: N/A

Related PR: #65668, #65669

Problem Summary:

In P0 build 996774, test_ddl_restore_auth submitted BACKUP SNAPSHOT at 19:40:11.661. The authorized DROP REPOSITORY in test_ddl_repository_auth started 26 ms later and exhausted BackupHandler's 10-second global submission-lock wait before the backup submission returned.

The authorized DROP now retries only the documented transient message "Another backup or restore job is being submitted", with at most 15 attempts and a 2-second interval. All other errors fail immediately.

The follow-up keeps test_ddl_restore_auth in the nonConcurrent SINGLE phase. Review of both lock directions showed that moving it back to NORMAL would also require exact transient handling for required fixed-name setup DROPs and expected-success BACKUP/RESTORE statements. Keeping the existing scheduling isolation is the smallest change that fully covers those orderings.

Release note

None

Check List (For Author)

  • Test

    • Regression test
    • Unit Test
    • Manual test
    • No need to test or manual test. Explain why:
      • Other reason: regression framework package build passed; the actual suite group was deterministically classified as SINGLE; both related Groovy files compiled; git diff --check passed. Runtime validation will be performed by CI.
  • Behavior changed:

    • No.
    • Yes.
  • Does this need documentation?

    • No.
    • Yes.

Check List (For Reviewer who merge this PR)

  • Confirm the release note
  • Confirm test cases
  • Confirm document
  • Add branch pick label

### What problem does this PR solve?

Issue Number: N/A

Related PR: N/A

Problem Summary: test_ddl_repository_auth can overlap a long backup submission from test_ddl_restore_auth. Its authorized DROP REPOSITORY then exhausts BackupHandler's 10-second submission-lock wait even though the authorization behavior is correct. Keep both suites concurrent and retry only the documented transient lock-contention error, matching the existing backup-auth retry strategy.

### Release note

None

### Check List (For Author)

- Test: No need to test (regression-only wait logic; git diff --check and exact net-diff inspection passed)
- Behavior changed: No
- Does this need documentation: No
@hello-stephen

Copy link
Copy Markdown
Contributor

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

  1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
  2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
  3. What features were added. Why was this function added?
  4. Which code was refactored and why was this part of the code refactored?
  5. Which functions were optimized and what is the difference before and after the optimization?

@shuke987

Copy link
Copy Markdown
Collaborator Author

/review

@shuke987

Copy link
Copy Markdown
Collaborator Author

run buildall

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Review result: Request changes

The targeted DROP retry handles the reproduced ordering, but removing nonConcurrent makes the restore suite concurrent with other repository tests while only one expected-success statement is retried. The inline issue identifies two remaining holder/waiter orderings.

Critical checkpoint conclusions:

  • Goal and proof: The PR aims to recover roughly seven minutes of serialized test time without reintroducing lock-contention flakes. The one-site retry proves only the originally observed DROP-waits-for-BACKUP path; it does not make the full newly concurrent shard safe, and no deterministic parallel/repeated-state run closes that gap.
  • Scope and focus: The textual edit is small, but removing the suite tag changes scheduling against every NORMAL auth suite. The implementation is therefore incomplete for its actual concurrency scope.
  • Concurrency: NORMAL suite workers are the entry point. BACKUP/RESTORE and DROP share BackupHandler.seqlock; DROP then takes RepositoryMgr.lock, while CREATE holds that manager lock across S3 initialization. This leaves both restore-as-holder and restore-as-waiter timeout orderings. No deadlock cycle was found because CREATE does not wait for seqlock.
  • Lifecycle and resource ownership: Restore-auth's randomized database, repository, label, and S3 path, plus its suite-unique fixed user, isolate its direct resources. The distinct retained-state problem is the fixed repository deliberately left by test_show_repository_auth, whose required setup DROP can be swallowed. No static-initialization issue applies.
  • Configuration and compatibility: No configuration, protocol, symbol, serialization, storage-format, rolling-upgrade, or FE/BE variable change is introduced.
  • Parallel and conditional paths: Unauthorized statements fail before BackupHandler; SHOW/CANCEL do not take seqlock; ALTER REPOSITORY is unused here; test_ddl_backup_auth remains in the later SINGLE phase. The required fixed-name setup DROPs and restore-auth's expected-success BACKUP/RESTORE are the uncovered NORMAL paths.
  • Error handling: The new exact-message classifier is sound on the direct JDBC path. Success breaks, nonmatching and final failures rethrow, interruption cannot silently pass, and the following negative SHOW verifies removal.
  • Tests and results: Existing authorization assertions remain meaningful and no .out result change applies, but there is no deterministic coverage for both lock orderings, suiteParallel > 1, or supported repeated/retained state. git diff --check passed. Per the review-runner contract, no build or regression test was run here; the external compile check was still pending at submission time.
  • Observability and performance: Retry warnings retain the JDBC message and terminal failures propagate. Uncontended overhead is negligible; continuous matching contention is bounded at about 178 seconds. No production metric is warranted for this regression-only path.
  • Persistence, transactions, and data writes: No production persistence or transaction behavior changes. Test DDL/DML remains isolated to the randomized database.
  • Other concerns: No separate memory, nullability, version, delete-bitmap, security, or privilege-semantics issue was found.

User focus: No additional review focus was provided.

import java.util.UUID

suite("test_ddl_restore_auth", "p0,auth_call,nonConcurrent") {
suite("test_ddl_restore_auth", "p0,auth_call") {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[P1] Cover both lock directions before enabling this suite

Moving this suite into the NORMAL pool still leaves supported executions vulnerable to the same timeout in both directions. CREATE REPOSITORY holds RepositoryMgr.lock across S3 initialization, so a concurrent patched DROP can acquire BackupHandler.seqlock and then block on that manager lock; this suite's unretried BACKUP/RESTORE can then time out after 10 seconds. Conversely, on --times > 1 or a retained cluster, test_show_repository_auth has a retained fixed repository, its try_sql(DROP) can time out behind this suite and be swallowed, and its next CREATE fails as already existing. The new loop protects only one DROP call. Please either keep nonConcurrent, or apply the exact transient retry to every required setup/drop plus this suite's expected-success BACKUP/RESTORE statements before parallelizing it.

### What problem does this PR solve?

Issue Number: N/A

Related PR: apache#65668, apache#65691

Problem Summary: Removing the nonConcurrent group exposes test_ddl_restore_auth to both directions of the global backup submission lock ordering and can also race with fixed-name repository setup on retained or repeated runs. Keep the long restore authorization suite in the SINGLE phase while retaining the targeted transient retry for repository DROP.

### Release note

None

### Check List (For Author)

- Test: Regression framework package build; deterministic SINGLE group classification assertion; Groovy syntax compilation; git diff --check

- Behavior changed: No

- Does this need documentation: No
@shuke987

Copy link
Copy Markdown
Collaborator Author

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Review result: Approval opinion (comment-only)

The current head addresses the earlier concurrency concern by keeping test_ddl_restore_auth in the nonConcurrent SINGLE phase, while the remaining one-file change narrowly retries the authorized repository DROP on the exact global backup-submission-lock transient. I found no new inline issue.

Critical checkpoint conclusions:

  • Goal and proof: The goal is to prevent test_ddl_repository_auth from failing when its required successful DROP overlaps another P0 backup/restore submission. BackupHandler.tryLock() is the sole producer of the matched text and emits it before repository lookup or mutation; the loop retries only that condition, rethrows every other/final failure, and the following negative SHOW CREATE REPOSITORY confirms removal. The suite itself covers the authorization and final-state behavior, although no deterministic lock-contention regression was added.
  • Scope and clarity: The authoritative net diff changes only test_ddl_repository_auth.groovy and is small and focused. The prior scheduling expansion was reverted, so current head does not broaden restore-auth concurrency.
  • Concurrency and locks: NORMAL suite workers can overlap other P0 backup/restore submitters on BackupHandler.seqlock. A failed tryLock(10s) cannot have entered RepositoryMgr.removeRepo() or its EditLog write, making the retry idempotent. Backup-auth and restore-auth remain SINGLE and start only after all NORMAL futures complete. No new shared test state, nested lock order, or deadlock path is introduced.
  • Error handling and conditions: The MySQL/JDBC/Groovy path propagates the FE error text without catch-and-translation. Success breaks; a nonmatching error, interruption error, or the fifteenth matching failure propagates. The exact-message condition is therefore specific to the known inevitable transient rather than a blanket retry.
  • Lifecycle and cleanup: The retry stays within the existing authenticated connection closure and is bounded at 15 attempts. Successful execution still proves and cleans up the suite-owned repository before database/user cleanup. No product lifecycle, static-initialization, ownership, or resource-cycle change applies.
  • Configuration and compatibility: No configuration, protocol, symbol, serialization, storage format, rolling-upgrade behavior, or FE/BE variable is changed.
  • Parallel paths and prior review context: The previous P1 requested either comprehensive retry coverage after parallelizing restore-auth or retaining nonConcurrent; current head implements the latter. The fixed-name dirty/retained-state setup concern is already covered by that thread and predates this patch, so it was not duplicated.
  • Tests and results: No .out result changes apply. The existing post-DROP negative assertion remains valid. Repository checks currently show style/title and other lightweight checks passing, with the automated code-review check pending. Per this review runner's instruction, I did not run builds or regression tests, so runtime validation remains with CI.
  • Observability and performance: Each transient retry logs the server message and terminal failures remain visible. Uncontended overhead is one attempt; continuous contention is bounded at about 178 seconds, below the framework's 30-minute JDBC socket timeout, and no per-suite timeout was found. No production metric is warranted for a regression-only retry.
  • Persistence, transactions, and data writes: No production transaction or persistence contract changes. On the reviewed path, the matched failure occurs before repository map mutation and logDropRepository(), so retry cannot duplicate a committed metadata change.
  • Other invariants: No memory tracking, nullability, visible-version, delete-bitmap, security-boundary, or privilege-semantics regression was found.

User focus: No additional review focus was provided.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants