[enhancement](cloud) batching get visible version from MetaService #34615
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. |
freemandealer
force-pushed
the
batch-get-version
branch
from
4af33e4
to
82fdecc
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! 👍" |
freemandealer
force-pushed
the
batch-get-version
branch
from
278e48b
to
c218ff0
Compare
|
run buildall |
|
clang-tidy review says "All clean, LGTM! 👍" |
gavinchou
reviewed
May 14, 2024
| @@ -253,16 +257,28 @@ private boolean tablesOrDataChanged(Env env, SqlCacheContext sqlCacheContext) { | |||
| return true; | |||
| } | |||
| OlapTable olapTable = (OlapTable) tableIf; | |||
| long currentTableTime = olapTable.getVisibleVersionTime(); | |||
| long cacheTableTime = scanTable.latestTimestamp; | |||
| //long currentTableTime = olapTable.getVisibleVersionTime(); | |||
There was a problem hiding this comment.
ok, will do after all these p0 cases passes.
gavinchou
requested changes
May 14, 2024
gensrc/proto/cloud.proto
Outdated
| @@ -662,6 +663,7 @@ message CommitTxnResponse { | |||
| repeated int64 partition_ids = 4; | |||
| repeated int64 versions = 5; | |||
| repeated TableStatsPB table_stats = 6; | |||
| optional int64 version_update_time = 7; | |||
There was a problem hiding this comment.
What is this for? Why is version_update_time related to RecycleTxnPB
There was a problem hiding this comment.
They are for batch and single respectively. What makes you think the relation ever existed?
gensrc/proto/cloud.proto
Outdated
| @@ -759,6 +761,8 @@ message GetVersionResponse { | |||
| repeated int64 table_ids = 4; | |||
| repeated int64 partition_ids = 5; | |||
| repeated int64 versions = 6; | |||
| repeated int64 version_update_times = 7; | |||
There was a problem hiding this comment.
what's the single and multiple update_time for?
| @@ -1043,10 +1043,14 @@ void MetaServiceImpl::commit_txn(::google::protobuf::RpcController* controller, | |||
| } | |||
|
|
|||
| // Save versions | |||
| int64_t version_update_time = | |||
There was a problem hiding this comment.
add time unit as suffix
e.g. version_update_time_ms
pls. check all the other occurrences
There was a problem hiding this comment.
sure, will do
morrySnow
reviewed
May 15, 2024
fe/fe-core/src/main/java/org/apache/doris/common/NereidsSqlCacheManager.java
Outdated
Show resolved
Hide resolved
Comment on lines
706
to
718
| if (Config.isCloudMode()) { | ||
| Collection<Long> partitionIds = node.getSelectedPartitionIds(); | ||
| List<CloudPartition> partitions = partitionIds.stream() | ||
| .sorted() | ||
| .map(olapTable::getPartition) | ||
| .map(partition -> (CloudPartition) partition) | ||
| .collect(Collectors.toList()); | ||
| try { | ||
| CloudPartition.getSnapshotVisibleVersion(partitions); | ||
| } catch (RpcException e) { | ||
| throw new RuntimeException(e); | ||
| } | ||
| } |
There was a problem hiding this comment.
add comment to explain it
|
clang-tidy review says "All clean, LGTM! 👍" |
1 similar comment
|
clang-tidy review says "All clean, LGTM! 👍" |
|
run buildall |
w41ter
reviewed
May 15, 2024
| @@ -261,6 +261,7 @@ void MetaServiceImpl::get_version(::google::protobuf::RpcController* controller, | |||
| return; | |||
| } | |||
| response->set_version(version); | |||
| // TODO response->set_version_update_time_ms(); | |||
There was a problem hiding this comment.
Suggested change
| // TODO response->set_version_update_time_ms(); |
| @@ -381,6 +383,7 @@ void MetaServiceImpl::batch_get_version(::google::protobuf::RpcController* contr | |||
| break; | |||
| } | |||
| response->add_versions(version); | |||
| // TODO response->add_version_update_times_ms(); | |||
There was a problem hiding this comment.
Suggested change
| // TODO response->add_version_update_times_ms(); |
| scanTables.add(scanTable); | ||
| if (Config.isCloudMode()) { | ||
| // get version of related partitions in batch |
There was a problem hiding this comment.
How about extracting this operation into OlapTable, just like OlapTable.selectNonEmptyPartitionIds?
There was a problem hiding this comment.
How about extracting this operation into
OlapTable, just likeOlapTable.selectNonEmptyPartitionIds?
The one in NereidssqlCacheManager doesn't use OlapTable but ScanTable, I have no handy idea to unify the two.
There was a problem hiding this comment.
Ops! On second thought, I think can give it a try.
TPC-H: Total hot run time: 40655 ms |
TPC-DS: Total hot run time: 186606 ms |
ClickBench: Total hot run time: 30.89 s |
w41ter
previously approved these changes
May 15, 2024
|
run buildall |
|
clang-tidy review says "All clean, LGTM! 👍" |
TPC-H: Total hot run time: 40406 ms |
TPC-DS: Total hot run time: 186773 ms |
ClickBench: Total hot run time: 31.09 s |
TPC-DS: Total hot run time: 171850 ms |
ClickBench: Total hot run time: 30.46 s |
gavinchou
reviewed
Jun 27, 2024
Get visible version one by one from MetaService is costly. Batching them into one RPC will not only reduce the work load of RPC service but also reduce the lag. Signed-off-by: freemandealer <[email protected]>
Signed-off-by: freemandealer <[email protected]>
Signed-off-by: freemandealer <[email protected]>
freemandealer
force-pushed
the
batch-get-version
branch
from
5100bef
to
8a147e6
Compare
|
run buildall |
|
clang-tidy review says "All clean, LGTM! 👍" |
TPC-H: Total hot run time: 39617 ms |
TPC-DS: Total hot run time: 173967 ms |
ClickBench: Total hot run time: 30.42 s |
|
run p0 |
2 similar comments
|
run p0 |
|
run p0 |
gavinchou
approved these changes
Jun 28, 2024
github-actions
bot
added
the
approved
|
PR approved by at least one committer and no changes requested. |
|
PR approved by anyone and no changes requested. |
Yukang-Lian
approved these changes
Jun 28, 2024
dataroaring
pushed a commit
that referenced
this pull request
Jun 30, 2024
…34615) Get visible versions one by one from MetaService is costly. Batching them into one RPC will not only reduce the workload of RPC service but also reduce the lag.
gavinchou
added a commit
that referenced
this pull request
Jul 1, 2024
This PR fix the in-compatibility introduced by #34615
dataroaring
pushed a commit
that referenced
this pull request
Jul 2, 2024
This PR fix the in-compatibility introduced by #34615
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.
Get visible versions one by one from MetaService is costly. Batching them into one RPC will not only reduce the workload of RPC service but also reduce the lag.