Skip to content
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.

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

fix: use cut-over-lock-timeout for instant DDL #1468

Merged
merged 5 commits into from
Dec 17, 2024

Conversation

artemvovk
Copy link
Contributor

@artemvovk artemvovk commented Nov 7, 2024

A Pull Request should be associated with an Issue.

Related issue: #1386

Description

reusing the cut-over-lock-timeout from the cutover code. The lock wait timeout in the original code is actually set to double the setting, so we keep that consistent. Altho I do wonder why it's 2x.

there are no existing tests for the CutOverLockTimeoutSeconds currently, I can add some if that would help.

In case this PR introduced Go code changes:

  • contributed code is using same conventions as original code
  • script/cibuild returns with no formatting errors, build errors or unit test errors.

Addresses github#1386

by reusing the cut-over-lock-timeout from the cutover code.
The lock wait timeout in the original code is actually set to double
the setting, so we keep that consistent.
@artemvovk artemvovk force-pushed the 1386-instant-lock-timeout branch from 43303a4 to b74154d Compare November 7, 2024 17:56
@arthurschreiber
Copy link
Member

I'm wondering if we shouldn't just set the lock timeout to the minimal possible value for instant ddl? 🤔

@artemvovk
Copy link
Contributor Author

artemvovk commented Nov 7, 2024

that would be my first choice; but also having it configurable doesn't seem like the worst idea - I can update either way, delegating the decision to the reviewers.

@meiji163
Copy link
Contributor

meiji163 commented Dec 17, 2024

Configurable lock timeout seems OK to me, I will merge this after fixing a bug

@meiji163 meiji163 merged commit 690b1e1 into github:master Dec 17, 2024
7 checks passed
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.

3 participants