-
Notifications
You must be signed in to change notification settings - Fork 329
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
feat: drop if exists #2859
feat: drop if exists #2859
Conversation
Signed-off-by: tison <[email protected]>
Signed-off-by: tison <[email protected]>
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## develop #2859 +/- ##
===========================================
- Coverage 84.74% 84.40% -0.35%
===========================================
Files 740 740
Lines 115767 115796 +29
===========================================
- Hits 98105 97735 -370
- Misses 17662 18061 +399 |
Maybe I should add some e2e tests. But let's first take a look at the impl direction. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rest LGTM, Let's add some sqlness tests.
Signed-off-by: tison <[email protected]>
Updated. PTAL again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
Drop table procedures cannot run concurrently because they acquire the same procedure lock key. The key locks on the table name. |
Is this an unstable case - https://github.com/GreptimeTeam/greptimedb/actions/runs/7095139148/job/19311581914? Since we pass at the first attempt. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I hereby agree to the terms of the GreptimeDB CLA
What's changed and what's your intention?
This closes #2794.
@zuston it happens that I have some time to review this issue, if you're working on this issue, feel free to take any value from my PR and prioritize your patch.
@sunng87 I noticed that we actually check table existence before submit DropTable DDL task. If so, we can simply filter drop table tasks in
StatemenExecutor::drop_table
and assert table exists for the following producer.Also, I'd like to confirm whether DDL task procedures can run concurrently. That is, if two
DROP TABLE t
can run concurrently witht
exists. If so, we may need some lock/sync when removing table metadata.Checklist
Refer to a related PR or issue link (optional)