-
Notifications
You must be signed in to change notification settings - Fork 409
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
Build table snapshot on write node for disaggregated read from S3 #6919
Conversation
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
3072251
to
61471a5
Compare
/run-all-tests |
61471a5
to
5ade3a4
Compare
/run-all-tests |
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.
Only some minor suggestions
/* const */ bool is_mpp_task = false; | ||
/* const */ bool is_root_mpp_task = false; | ||
/* const */ bool is_batch_cop = false; | ||
/* const */ bool is_disaggregated_task = false; |
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.
Do we use this flag for both: requests in rn and establish requests in wn?
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.
The rn does not need to create a DAGContext with is_disaggregated_task==true
as it only needs to send disaggregated task. This flag is only set when a wn receives a disaggregated task
db_context->setMockStorage(mock_storage); | ||
db_context->setMockMPPServerInfo(mpp_test_info); | ||
|
||
RUNTIME_CHECK(context->isDisaggregatedStorageMode()); |
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.
Off topic: When will we use context and when will we use db_context?
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.
context
- actually it is a pointer to the global context
db_context
- a temporary context that its lifetime lasts only until the end of this 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.
db_context
is a session-only context, some value of db_context->getSettings()
is not the same as the global settings.
dbms/src/Storages/DeltaMerge/Remote/DisaggregatedSnapshotManager.h
Outdated
Show resolved
Hide resolved
dbms/src/Storages/DeltaMerge/Remote/DisaggregatedSnapshotManager.h
Outdated
Show resolved
Hide resolved
74249e6
to
772ed15
Compare
/run-all-tests |
// Note that `buildRemoteRequests` must be called after `buildLocalStreams` because | ||
// `buildLocalStreams` will setup `region_retry_from_local_region` and we must | ||
// retry those regions or there will be data lost. |
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.
detect by test case "tests/fullstack-test/fault-inject/exception_after_read_from_storage.test"
/cc @breezewish
Signed-off-by: JaySon-Huang <jayson.hjs@gmail.com>
Signed-off-by: JaySon-Huang <jayson.hjs@gmail.com>
Signed-off-by: JaySon-Huang <jayson.hjs@gmail.com>
Signed-off-by: JaySon-Huang <tshent@qq.com>
Signed-off-by: JaySon-Huang <tshent@qq.com>
Signed-off-by: JaySon-Huang <tshent@qq.com>
Signed-off-by: JaySon-Huang <tshent@qq.com>
Signed-off-by: JaySon-Huang <tshent@qq.com>
Signed-off-by: JaySon-Huang <jayson.hjs@gmail.com>
Signed-off-by: JaySon-Huang <jayson.hjs@gmail.com>
Signed-off-by: JaySon-Huang <jayson.hjs@gmail.com>
Signed-off-by: JaySon-Huang <jayson.hjs@gmail.com>
d41b5f3
to
c886f7d
Compare
/merge |
@JaySon-Huang: It seems you want to merge this PR, I will help you trigger all the tests: /run-all-tests You only need to trigger If you have any questions about the PR merge process, please refer to pr process. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
This pull request has been accepted and is ready to merge. Commit hash: c886f7d
|
/run-integration-test |
1 similar comment
/run-integration-test |
/run-all-tests |
/run-all-tests |
/run-integration-test |
What problem does this PR solve?
Issue Number: ref #6827
Problem Summary:
What is changed and how it works?
Depends on:
Changes:
DisaggregatedTaskId
for the task id, it is based on MPPId and executor_idDAGContext::is_disaggregated_task
DAGContext::disaggregated_id
DisaggregatedReadSnapshot
to hold the segment snapshot withDisaggregatedTaskId
DisaggregatedSnapshotManager
to hold the snapshots of logical table in the write nodeFlashService::EstablishDisaggregatedTask
DAGStorageInterpreter::buildLocalStreams
DisaggregatedSnapshotManager
in the write nodeEstablishDisaggregatedTaskResponse
FlashService::FetchDisaggregatedPages
DisaggregatedSnapshotManager
if the read node doesn't try to finish/cancel the requestCheck List
Tests
Side effects
Documentation
Release note