-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[fix](vault) Fix bugs about altering storage vault name #45685
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
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
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.
clang-tidy made some suggestions
| } | ||
|
|
||
| static int alter_s3_storage_vault(InstanceInfoPB& instance, std::unique_ptr<Transaction> txn, | ||
| static int alter_s3_storage_vault(InstanceInfoPB& instance, std::unique_ptr<Transaction>& txn, |
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.
warning: function 'alter_s3_storage_vault' exceeds recommended size/complexity thresholds [readability-function-size]
static int alter_s3_storage_vault(InstanceInfoPB& instance, std::unique_ptr<Transaction>& txn,
^Additional context
cloud/src/meta-service/meta_service_resource.cpp:654: 130 lines including whitespace and comments (threshold 80)
static int alter_s3_storage_vault(InstanceInfoPB& instance, std::unique_ptr<Transaction>& txn,
^
TPC-H: Total hot run time: 39897 ms |
TPC-DS: Total hot run time: 196967 ms |
ClickBench: Total hot run time: 33.53 s |
d5cd146 to
200e666
Compare
|
run buildall |
200e666 to
1dcce20
Compare
|
run buildall |
TPC-H: Total hot run time: 39758 ms |
TPC-DS: Total hot run time: 195955 ms |
ClickBench: Total hot run time: 33.79 s |
1dcce20 to
49f85f9
Compare
|
run buildall |
TPC-H: Total hot run time: 40243 ms |
TPC-DS: Total hot run time: 196911 ms |
49f85f9 to
70c82aa
Compare
|
run buildall |
TPC-H: Total hot run time: 39852 ms |
TPC-DS: Total hot run time: 189786 ms |
ClickBench: Total hot run time: 32.43 s |
gensrc/proto/cloud.proto
Outdated
| optional bool default_storage_vault_replaced = 3; | ||
|
|
||
| // storage_vault_name maybe changed, so return new storage_vault_name | ||
| optional string storage_vault_name = 4; |
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.
redundant vault name, using the name in request
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.
redundant vault name, using the name in request
done
| return false; | ||
| } | ||
|
|
||
| static int alter_hdfs_storage_vault(InstanceInfoPB& instance, std::unique_ptr<Transaction>& txn, |
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.
add UT
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.
add UT
done
* fix altering storage name but not writing disk in meta-service * check vault if existed when altering stoarge vault name
70c82aa to
a2a1c3b
Compare
|
run buildall |
TPC-H: Total hot run time: 32408 ms |
TPC-DS: Total hot run time: 196210 ms |
ClickBench: Total hot run time: 31.24 s |
|
PR approved by at least one committer and no changes requested. |
|
PR approved by anyone and no changes requested. |
| "VAULT_NAME" = "${hdfsVaultName}" | ||
| ); | ||
| """ | ||
| }, "vault name no change") |
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.
vault name no change -> unable to use the original vault name?
MoanasDaddyXu
left a comment
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
dataroaring
left a comment
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
* fix altering storage name but not writing disk in meta-service * check vault if existed when altering stoarge vault name
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)