-
Notifications
You must be signed in to change notification settings - Fork 411
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
ExchangeSenderOp check writer status only if there is data to flush to MPPTunnel #9736
base: master
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/test pull-unit-test |
/test pull-integration-test |
OperatorStatus ExchangeSenderSinkOp::tryFlush() | ||
{ | ||
assert(buffer); | ||
auto res = waitForWriter(); |
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.
Perhaps moving waitForWriter
into Writer::write
and Writer::flush
would be better. This way, even if the tunnel is busy, write can still batch in memory.
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.
You mean if the block's size is small, it will be cached in the writer without writting to tunnel?
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.
yes, if the total block rows is smaller than batch_send_min_limit
, MPPTunnel::write would not be call.
https://github.com/pingcap/tiflash/blob/master/dbms/src/Flash/Mpp/BroadcastOrPassThroughWriter.cpp#L103-L109
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.
Good idea, I reimplement this pr by moving waitForWriter
to Writer::flush
. But waitForWriter
is still needed in ExchangeSenderSinkOp::await
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
Signed-off-by: xufei <[email protected]>
Signed-off-by: xufei <[email protected]>
Signed-off-by: xufei <[email protected]>
Signed-off-by: xufei <[email protected]>
43283c9
to
f2b866d
Compare
Signed-off-by: xufei <[email protected]>
Signed-off-by: xufei <[email protected]>
Signed-off-by: xufei <[email protected]>
Signed-off-by: xufei <[email protected]>
Signed-off-by: xufei <[email protected]>
Signed-off-by: xufei <[email protected]>
What problem does this PR solve?
Issue Number: ref #6233
Problem Summary:
Currently,
ExchangeSenderSinkOp
check the writer status inExchangeSenderSinkOp::prepareImpl()
, so even if there is currently no data to write, theExchangeSenderSinkOp
need to check the writer status, and the pipeline task maybe blocked if the writer is not ready. In this pr,ExchangeSenderSinkOp
only check the writer status if it has data to flush to MPPTunnel.What is changed and how it works?
Check List
Tests
Side effects
Documentation
Release note