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: show locks in current tenant #15252

Merged
merged 3 commits into from
Apr 17, 2024
Merged

Conversation

zhyass
Copy link
Member

@zhyass zhyass commented Apr 17, 2024

I hereby agree to the terms of the CLA available at: https://docs.databend.com/dev/policies/cla/

Summary

Add tenant in lock key, show locks only in the current tenant.

__fd_table_lock/<tenant>/table_id/revision -> LockMeta

This is an incompatible modification. It is recommended to wait for existing table locks to time out for normal upgrades (second level).

  • Fixes #[Link the issue here]

Tests

  • Unit Test
  • Logic Test
  • Benchmark Test
  • No Test - Explain why

Type of change

  • Bug Fix (non-breaking change which fixes an issue)
  • New Feature (non-breaking change which adds functionality)
  • Breaking Change (fix or feature that could cause existing functionality not to work as expected)
  • Documentation Update
  • Refactoring
  • Performance Improvement
  • Other (please describe):

This change is Reviewable

@github-actions github-actions bot added the pr-bugfix this PR patches a bug in codebase label Apr 17, 2024
Copy link
Member

@drmingdrmer drmingdrmer left a comment

Choose a reason for hiding this comment

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

Reviewed 7 of 7 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @zhyass)


src/meta/app/src/schema/lock.rs line 149 at r1 (raw file):

        if self.table_ids.is_empty() {
            let table_lock_prefix =
                format!("{}/{}/", TableLockKey::PREFIX, self.tenant.tenant_name());

Do no use ::PREFIX directly, use the structured key to build a prefix with:

let ident = TableLockKey::new(self.tenant.clone(), 0, 0);
let prefix = DirName::new_with_level(lock, 2).dir_name_with_slash();

And it is prone to bug using this method to build both two levels prefix string and one level prefix string.
There should be another method to build a one level prefix string.

Copy link
Member

@drmingdrmer drmingdrmer left a comment

Choose a reason for hiding this comment

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

Reviewable status: 4 of 7 files reviewed, 1 unresolved discussion (waiting on @zhyass)


src/meta/app/src/schema/lock.rs line 149 at r1 (raw file):

Previously, drmingdrmer (张炎泼) wrote…

Do no use ::PREFIX directly, use the structured key to build a prefix with:

let ident = TableLockKey::new(self.tenant.clone(), 0, 0);
let prefix = DirName::new_with_level(lock, 2).dir_name_with_slash();

And it is prone to bug using this method to build both two levels prefix string and one level prefix string.
There should be another method to build a one level prefix string.

👆🏻This method builds two different kinds of keys. There should be two methods for two kinds of keys.

Copy link
Member

@drmingdrmer drmingdrmer left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 2 files at r3, 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @zhyass)

Copy link
Member Author

@zhyass zhyass left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @drmingdrmer)


src/meta/app/src/schema/lock.rs line 149 at r1 (raw file):

Previously, drmingdrmer (张炎泼) wrote…

Do no use ::PREFIX directly, use the structured key to build a prefix with:

let ident = TableLockKey::new(self.tenant.clone(), 0, 0);
let prefix = DirName::new_with_level(lock, 2).dir_name_with_slash();

And it is prone to bug using this method to build both two levels prefix string and one level prefix string.
There should be another method to build a one level prefix string.

Done.

@zhyass zhyass added this pull request to the merge queue Apr 17, 2024
Merged via the queue into datafuselabs:main with commit 8083914 Apr 17, 2024
72 of 73 checks passed
@zhyass zhyass deleted the feature_fix branch April 17, 2024 11:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-bugfix this PR patches a bug in codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants