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

refactor(query): add config enable_meta_data_upgrade_json_to_pb_from_v307 #16306

Merged
merged 9 commits into from
Aug 23, 2024

Conversation

TCeason
Copy link
Collaborator

@TCeason TCeason commented Aug 21, 2024

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

Summary

enable_meta_data_upgrade_json_to_pb_from_v307 default disable.

Old version upgrade to new version, not auto upgrade json to pb.

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-refactor this PR changes the code base without new features or bugfix label Aug 21, 2024
enable_upgrade_meta_data_to_pb default disable.

Old version upgrade to new version, not auto upgrade json to pb.
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 3 of 17 files at r1, all commit messages.
Reviewable status: 3 of 17 files reviewed, 1 unresolved discussion (waiting on @TCeason)


src/query/management/src/quota/quota_mgr.rs line 57 at r1 (raw file):

#[async_trait::async_trait]
impl<const WRITE_PB: bool> QuotaApi for QuotaMgr<WRITE_PB> {

It looks like QuotaMgr already have this flag in this generic const parameter WRITE_PB.
Set this const with the value of enable_upgrade_meta_data_to_pb would be just enough for your need IMHO.

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: 3 of 17 files reviewed, 2 unresolved discussions (waiting on @TCeason)


src/query/users/src/user_api.rs line 116 at r1 (raw file):

        conf: RpcClientConf,
        tenant: &Tenant,
        enable_upgrade_meta_data_to_pb: bool,

This flag should not be a flag for UserApiProvider, which is a container of various API managers .

The flag should only be added to the role_api method below.

Unless enable_upgrade_meta_data_to_pb will affect the behavior of all of the API managers in UserApiProvider.

Or introduce a dedicated Config instance as the third argument, which then defines configurations for each API manager.

@TCeason
Copy link
Collaborator Author

TCeason commented Aug 22, 2024

Reviewable status: 3 of 17 files reviewed, 2 unresolved discussions (waiting on @TCeason)


src/query/users/src/user_api.rs line 116 at r1 (raw file):

        conf: RpcClientConf,
        tenant: &Tenant,
        enable_upgrade_meta_data_to_pb: bool,

This flag should not be a flag for UserApiProvider, which is a container of various API managers .

The flag should only be added to the role_api method below.

Unless enable_upgrade_meta_data_to_pb will affect the behavior of all of the API managers in UserApiProvider.

Or introduce a dedicated Config instance as the third argument, which then defines configurations for each API manager.

Maybe Quota not support deserialize json? So I don't let this config control quota.

Now one time just upgrade 100 kvs
@TCeason TCeason changed the title refactor(query): add config enable_upgrade_meta_data_to_pb refactor(query): add config enable_meta_data_upgrade_json_to_pb_from_v307 Aug 22, 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 20 of 20 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @BohuTANG, @dantengsky, and @TCeason)


src/query/management/src/role/role_mgr.rs line 62 at r3 (raw file):

    kv_api: Arc<dyn kvapi::KVApi<Error = MetaError> + Send + Sync>,
    tenant: Tenant,
    enable_meta_data_upgrade_json_to_pb_from_v307: bool,

This flag does not have to be business specific at this level. It just upgrade_to_pb to RoleMgr.


src/query/management/src/role/role_mgr.rs line 534 at r3 (raw file):

fn quota(target: impl ToString, enable_meta_data_upgrade_json_to_pb_from_v307: bool) -> Quota {
    if enable_meta_data_upgrade_json_to_pb_from_v307 {

simplify the flag to enable_upgrade_to_pb.

Code quote:

fn quota(target: impl ToString, enable_meta_data_upgrade_json_to_pb_from_v307: bool) -> Quota {
    if enable_meta_data_upgrade_json_to_pb_from_v307 {

src/query/users/src/user_api.rs line 134 at r3 (raw file):

            GlobalConfig::instance()
                .query
                .enable_meta_data_upgrade_json_to_pb_from_v307,

This flag just specifies if to upgrade json to pb, although it is used for v307.

The name should reflect the purpose upgrade_to_pb, for from v307 could be part of its comment.

@TCeason
Copy link
Collaborator Author

TCeason commented Aug 22, 2024

enable_meta_data_upgrade_json_to_pb_from_v307

Done

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 3 of 4 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @BohuTANG, @dantengsky, and @TCeason)

@TCeason TCeason requested a review from Xuanwo August 23, 2024 03:06
@BohuTANG BohuTANG merged commit 2133a27 into databendlabs:main Aug 23, 2024
69 of 70 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-refactor this PR changes the code base without new features or bugfix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants