[feature-wip](Cloud) Support set default storage vault and pass vault id to all write paths #32910
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thank you for your contribution to Apache Doris. Since 2024-03-18, the Document has been moved to doris-website. |
ByteYue
force-pushed
the
use_default_vault
branch
from
8c6e386
to
b69b26f
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
3 similar comments
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
ByteYue
force-pushed
the
use_default_vault
branch
from
77e4237
to
cf3849f
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
2 similar comments
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
ByteYue
force-pushed
the
use_default_vault
branch
from
7dc41f3
to
147df6d
Compare
gavinchou
reviewed
Mar 29, 2024
gensrc/proto/cloud.proto
Outdated
| @@ -1379,6 +1398,8 @@ service MetaService { | |||
|
|
|||
| // routine load progress | |||
| rpc get_rl_task_commit_attach(GetRLTaskCommitAttachRequest) returns (GetRLTaskCommitAttachResponse); | |||
|
|
|||
| rpc get_default_vault(GetDefaultVaultRequest) returns (GetDefaultVaultResponse); | |||
gavinchou
requested changes
Mar 29, 2024
|
clang-tidy review says "All clean, LGTM! 👍" |
2 similar comments
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -5426,6 +5426,210 @@ TEST(MetaServiceTest, DropHdfsInfoTest) { | |||
| SyncPoint::get_instance()->clear_all_call_backs(); | |||
| } | |||
|
|
|||
| TEST(MetaServiceTest, GetDefaultVaultTest) { | |||
There was a problem hiding this comment.
warning: function 'TEST' exceeds recommended size/complexity thresholds [readability-function-size]
TEST(MetaServiceTest, GetDefaultVaultTest) {
^Additional context
cloud/test/meta_service_test.cpp:5428: 99 lines including whitespace and comments (threshold 80)
TEST(MetaServiceTest, GetDefaultVaultTest) {
^| hdfs.mutable_build_conf()->CopyFrom(conf); | ||
| req.mutable_hdfs_info()->CopyFrom(hdfs); | ||
|
|
||
| auto sp = SyncPoint::get_instance(); |
There was a problem hiding this comment.
warning: 'auto sp' can be declared as 'auto *sp' [readability-qualified-auto]
Suggested change
| auto sp = SyncPoint::get_instance(); | |
| auto *sp = SyncPoint::get_instance(); |
| instance.add_obj_info()->CopyFrom(obj_info); | ||
| req.mutable_obj_info()->CopyFrom(obj_info); | ||
|
|
||
| auto sp = SyncPoint::get_instance(); |
There was a problem hiding this comment.
warning: 'auto sp' can be declared as 'auto *sp' [readability-qualified-auto]
Suggested change
| auto sp = SyncPoint::get_instance(); | |
| auto *sp = SyncPoint::get_instance(); |
| } | ||
| } | ||
|
|
||
| TEST(MetaServiceTest, SetDefaultVaultTest) { |
There was a problem hiding this comment.
warning: function 'TEST' exceeds recommended size/complexity thresholds [readability-function-size]
TEST(MetaServiceTest, SetDefaultVaultTest) {
^Additional context
cloud/test/meta_service_test.cpp:5529: 101 lines including whitespace and comments (threshold 80)
TEST(MetaServiceTest, SetDefaultVaultTest) {
^| hdfs.mutable_build_conf()->CopyFrom(conf); | ||
| req.mutable_hdfs_info()->CopyFrom(hdfs); | ||
|
|
||
| auto sp = SyncPoint::get_instance(); |
There was a problem hiding this comment.
warning: 'auto sp' can be declared as 'auto *sp' [readability-qualified-auto]
Suggested change
| auto sp = SyncPoint::get_instance(); | |
| auto *sp = SyncPoint::get_instance(); |
| // Try to set one non-existent vault as default | ||
| { | ||
| StorageVaultPB hdfs; | ||
| auto name = "test_alter_add_hdfs_info_no"; |
There was a problem hiding this comment.
warning: 'auto name' can be declared as 'const auto *name' [readability-qualified-auto]
Suggested change
| auto name = "test_alter_add_hdfs_info_no"; | |
| const auto *name = "test_alter_add_hdfs_info_no"; |
|
run buildall |
TPC-H: Total hot run time: 38186 ms |
TPC-DS: Total hot run time: 182617 ms |
ClickBench: Total hot run time: 29.62 s |
|
TeamCity be ut coverage result: |
|
run buildall |
|
TeamCity be ut coverage result: |
TPC-H: Total hot run time: 38247 ms |
TPC-DS: Total hot run time: 182768 ms |
ClickBench: Total hot run time: 29.33 s |
ByteYue
force-pushed
the
use_default_vault
branch
from
78b6819
to
c1175cb
Compare
|
run buildall |
TPC-H: Total hot run time: 39315 ms |
TPC-DS: Total hot run time: 181692 ms |
|
TeamCity be ut coverage result: |
ClickBench: Total hot run time: 31.11 s |
|
Load test result on machine: 'aliyun_ecs.c7a.8xlarge_32C64G' |
gavinchou
reviewed
Apr 1, 2024
| }); | ||
|
|
||
| if (s3_obj != instance.obj_info().end()) { | ||
| response->set_storage_vault_id(s3_obj->id()); |
There was a problem hiding this comment.
should we also just set storage_vault_id in tablet meta?
|
run buildall |
TPC-H: Total hot run time: 39031 ms |
TPC-DS: Total hot run time: 181070 ms |
ClickBench: Total hot run time: 29.52 s |
|
TeamCity be ut coverage result: |
|
Load test result on machine: 'aliyun_ecs.c7a.8xlarge_32C64G' |
gavinchou
approved these changes
Apr 1, 2024
| } while (true); | ||
|
|
||
| CHECK(!vault_infos.empty()) << "no vault infos"; | ||
| } while (vault_infos.empty()); |
There was a problem hiding this comment.
while true and remember to clear vault_info each iteration
|
PR approved by anyone and no changes requested. |
github-actions
bot
added
the
approved
|
PR approved by at least one committer and no changes requested. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proposed changes
Issue Number: close #xxx
Further comments
If this is a relatively large or complex change, kick off the discussion at [email protected] by explaining why you chose the solution you did and what alternatives you considered, etc...