.*: refine streaming writer and exchange writer #6186

SeaRise · 2022-10-26T08:59:33Z

What problem does this PR solve?

Issue Number: ref #5900

Problem Summary:

What is changed and how it works?

change void MPPTunnelSet::write(mpp::MPPDataPacket & packet); to void MPPTunnelSet::write(const TrackedMppDataPacketPtr & packet);
remove useless function of StreamWriter and MPPTunnelSet
Detach sending execution summary from writer::write(), and always sending execution summary packet separately in writer::finishWrite.
add function void sendExecutionSummary(tipb::SelectResponse & response); for MPPTunnelSet to send execution summary specifically.
Rename the function of MPPTunnelSet to write, partitionWrite and broadcastWrite
copy MPPDataPacket in MPPTunnelSet::broadcastWrite or not MPPTunnelWriter::write

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

None

ti-chi-bot · 2022-10-26T08:59:34Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

windtalker
ywqzzy

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

SeaRise · 2022-10-26T09:30:44Z

/run-all-tests

SeaRise · 2022-10-26T10:18:32Z

/run-all-tests

SeaRise · 2022-10-26T10:22:30Z

dbms/src/Flash/Mpp/MPPTunnelSet.cpp

@@ -33,76 +33,17 @@ void checkPacketSize(size_t size)
 } // namespace

 template <typename Tunnel>
-void MPPTunnelSetBase<Tunnel>::clearExecutionSummaries(tipb::SelectResponse & response)


The execution summary is guaranteed by the upper-layer. No need to check again.

dbms/src/Flash/Mpp/BroadcastOrPassThroughWriter.cpp

SeaRise · 2022-10-26T11:17:36Z

/run-all-tests

dbms/src/Flash/Mpp/BroadcastOrPassThroughWriter.cpp

windtalker

LGTM

yibin87

LGTM

SeaRise · 2022-11-09T02:56:12Z

/merge

ti-chi-bot · 2022-11-09T02:56:13Z

@SeaRise: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

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.

ti-chi-bot · 2022-11-09T02:56:29Z

@SeaRise: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

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.

SeaRise · 2022-11-09T03:54:02Z

/hold

SeaRise · 2022-11-09T04:15:51Z

/hold cancel

SeaRise · 2022-11-09T04:15:57Z

/merge

ti-chi-bot · 2022-11-09T04:15:58Z

@SeaRise: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

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.

SeaRise · 2022-11-09T04:16:46Z

/merge cancel

SeaRise · 2022-11-09T04:18:45Z

/merge

ti-chi-bot · 2022-11-09T04:18:46Z

@SeaRise: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

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.

ti-chi-bot · 2022-11-09T04:18:48Z

This pull request has been accepted and is ready to merge.

Commit hash: 4177fa2

refine mpptunnel.write

95c4725

ti-chi-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 26, 2022

fix ut

94d4562

SeaRise changed the title ~~[WIP] .*: refine mpptunnel.write~~ .*: refine mpptunnel.write Oct 26, 2022

ti-chi-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 26, 2022

update

ad8079d

ti-chi-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 26, 2022

SeaRise commented Oct 26, 2022

View reviewed changes

SeaRise changed the title ~~.*: refine mpptunnel.write~~ [WIP] .*: refine mpptunnel.write Oct 26, 2022

ti-chi-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 26, 2022

update

531574c

SeaRise changed the title ~~[WIP] .*: refine mpptunnel.write~~ .*: refine mpptunnel.write Oct 26, 2022

ti-chi-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 26, 2022

SeaRise changed the title ~~.*: refine mpptunnel.write~~ [WIP] .*: refine mpptunnel.write Oct 26, 2022

ti-chi-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 26, 2022

u

49718cc

ti-chi-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 26, 2022

u

a4ba98c

SeaRise commented Oct 26, 2022

View reviewed changes

dbms/src/Flash/Mpp/BroadcastOrPassThroughWriter.cpp Outdated Show resolved Hide resolved

SeaRise changed the title ~~[WIP] .*: refine mpptunnel.write~~ .*: refine mpptunnel.write Oct 26, 2022

ti-chi-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 26, 2022

SeaRise changed the title ~~.*: refine mpptunnel.write~~ .*: refine MPPTunnelSet.write Oct 26, 2022

rename

96abf0c

SeaRise requested a review from windtalker November 8, 2022 06:19

windtalker approved these changes Nov 9, 2022

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Nov 9, 2022

yibin87 approved these changes Nov 9, 2022

View reviewed changes

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Nov 9, 2022

Merge branch 'master' into refine_mpp_tunnel

d5f1010

ti-chi-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 9, 2022

SeaRise added 2 commits November 9, 2022 11:59

fix ut TestTiRemoteBlockInputStream

77232f0

fix ut

61e8647

ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Nov 9, 2022

ti-chi-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 9, 2022

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Nov 9, 2022

ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Nov 9, 2022

remove debug code

4177fa2

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Nov 9, 2022

ti-chi-bot merged commit 16cf524 into pingcap:master Nov 9, 2022

SeaRise deleted the refine_mpp_tunnel branch November 9, 2022 05:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

.*: refine streaming writer and exchange writer #6186

.*: refine streaming writer and exchange writer #6186

SeaRise commented Oct 26, 2022 •

edited

Loading

ti-chi-bot commented Oct 26, 2022 •

edited

Loading

SeaRise commented Oct 26, 2022

SeaRise commented Oct 26, 2022

SeaRise Oct 26, 2022 •

edited

Loading

SeaRise commented Oct 26, 2022

windtalker left a comment

yibin87 left a comment

SeaRise commented Nov 9, 2022

ti-chi-bot commented Nov 9, 2022

ti-chi-bot commented Nov 9, 2022

SeaRise commented Nov 9, 2022

SeaRise commented Nov 9, 2022

SeaRise commented Nov 9, 2022

ti-chi-bot commented Nov 9, 2022

SeaRise commented Nov 9, 2022

SeaRise commented Nov 9, 2022

ti-chi-bot commented Nov 9, 2022

ti-chi-bot commented Nov 9, 2022

.*: refine streaming writer and exchange writer #6186

.*: refine streaming writer and exchange writer #6186

Conversation

SeaRise commented Oct 26, 2022 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Oct 26, 2022 • edited Loading

SeaRise commented Oct 26, 2022

SeaRise commented Oct 26, 2022

SeaRise Oct 26, 2022 • edited Loading

Choose a reason for hiding this comment

SeaRise commented Oct 26, 2022

windtalker left a comment

Choose a reason for hiding this comment

yibin87 left a comment

Choose a reason for hiding this comment

SeaRise commented Nov 9, 2022

ti-chi-bot commented Nov 9, 2022

ti-chi-bot commented Nov 9, 2022

SeaRise commented Nov 9, 2022

SeaRise commented Nov 9, 2022

SeaRise commented Nov 9, 2022

ti-chi-bot commented Nov 9, 2022

SeaRise commented Nov 9, 2022

SeaRise commented Nov 9, 2022

ti-chi-bot commented Nov 9, 2022

ti-chi-bot commented Nov 9, 2022

SeaRise commented Oct 26, 2022 •

edited

Loading

ti-chi-bot commented Oct 26, 2022 •

edited

Loading

SeaRise Oct 26, 2022 •

edited

Loading