add spiller #6568

windtalker · 2023-01-03T05:56:17Z

Signed-off-by: xufei xufei@pingcap.com

What problem does this PR solve?

Issue Number: ref #6528

Problem Summary:

What is changed and how it works?

add Spiller to handle spill/restore blocks.

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 · 2023-01-03T05:56:18Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

SeaRise
mengxin9014

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.

dbms/src/Common/Spiller.cpp

dbms/src/Common/Spiller.h

dbms/src/Common/Spiller.cpp

dbms/src/Common/Spiller.h

dbms/src/Common/tests/gtest_spiller.cpp

dbms/src/Common/Spiller.h

dbms/src/Common/Spiller.cpp

dbms/src/Common/tests/gtest_spiller.cpp

dbms/src/DataStreams/SpilledFilesInputStream.cpp

dbms/src/Common/tests/gtest_spiller.cpp

Signed-off-by: xufei <xufei@pingcap.com>

windtalker · 2023-01-05T07:32:12Z

/run-unit-test

SeaRise

Others LGTM

SeaRise · 2023-01-05T08:21:17Z

dbms/src/Common/Spiller.cpp

+    {
+        size_t return_stream_num = std::min(max_stream_size, spilled_files[partition_id]->spilled_files.size());
+        std::vector<std::vector<String>> files(return_stream_num);
+        for (size_t i = 0; i < spilled_files[partition_id]->spilled_files.size(); ++i)


Maybe we can add todo here.
TODO balance based on SpilledDataSize

dbms/src/Common/Spiller.cpp

SeaRise · 2023-01-05T08:26:43Z

dbms/src/Common/Spiller.cpp

+            remove(true);
+    }
+    catch (...)
+    {


add some log here?
This way, if there is a problem with cleaning, we can know where the files are not being cleaned and clean them manually

Signed-off-by: xufei <xufei@pingcap.com>

windtalker · 2023-01-06T01:12:18Z

/run-all-tests

SeaRise

Others LGTM

dbms/src/Common/Spiller.cpp

SeaRise · 2023-01-06T02:32:35Z

dbms/src/Common/Spiller.cpp

+
+static Poco::Logger * getSpilledFileLogger()
+{
+    static Poco::Logger * logger = &Poco::Logger::get("SpilledFile");


I prefer to use DB::Logger

Signed-off-by: xufei <xufei@pingcap.com>

mengxin9014

LGTM

mengxin9014 · 2023-01-06T03:00:01Z

/run-all-tests

windtalker · 2023-01-06T03:37:18Z

/merge

ti-chi-bot · 2023-01-06T03:37:19Z

@windtalker: 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 · 2023-01-06T03:37:20Z

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

Commit hash: d31dfcc

ti-chi-bot · 2023-01-06T03:37:33Z

@windtalker: 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

trigger some heavy tests which will not run always when PR updated.

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.

ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 3, 2023

mengxin9014 reviewed Jan 3, 2023

View reviewed changes

windtalker force-pushed the spiller branch from 3452f66 to 7c7d18d Compare January 4, 2023 01:57

SeaRise reviewed Jan 4, 2023

View reviewed changes

windtalker force-pushed the spiller branch from 311eb9b to e3fc04d Compare January 4, 2023 08:27

SeaRise self-requested a review January 4, 2023 08:36

SeaRise reviewed Jan 4, 2023

View reviewed changes

windtalker added 5 commits January 5, 2023 14:45

add spiller

5d88961

Signed-off-by: xufei <xufei@pingcap.com>

little fix

f7ed163

Signed-off-by: xufei <xufei@pingcap.com>

address comments

08aa421

Signed-off-by: xufei <xufei@pingcap.com>

address comments

b00d0ce

Signed-off-by: xufei <xufei@pingcap.com>

address comments

7822478

Signed-off-by: xufei <xufei@pingcap.com>

windtalker force-pushed the spiller branch from e3fc04d to 7822478 Compare January 5, 2023 07:25

SeaRise self-requested a review January 5, 2023 08:16

SeaRise reviewed Jan 5, 2023

View reviewed changes

address comments

8bf4bce

Signed-off-by: xufei <xufei@pingcap.com>

SeaRise approved these changes Jan 6, 2023

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Jan 6, 2023

address comments

d31dfcc

Signed-off-by: xufei <xufei@pingcap.com>

mengxin9014 approved these changes Jan 6, 2023

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 Jan 6, 2023

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jan 6, 2023

Merge branch 'master' into spiller

acf20ce

ti-chi-bot merged commit b930bd8 into pingcap:master Jan 6, 2023

windtalker deleted the spiller branch January 30, 2023 12:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add spiller #6568

add spiller #6568

windtalker commented Jan 3, 2023

ti-chi-bot commented Jan 3, 2023 •

edited

Loading

windtalker commented Jan 5, 2023

SeaRise left a comment

SeaRise Jan 5, 2023

windtalker Jan 6, 2023

SeaRise Jan 5, 2023

windtalker Jan 6, 2023

windtalker commented Jan 6, 2023

SeaRise left a comment

SeaRise Jan 6, 2023

windtalker Jan 6, 2023

mengxin9014 left a comment

mengxin9014 commented Jan 6, 2023

windtalker commented Jan 6, 2023

ti-chi-bot commented Jan 6, 2023

ti-chi-bot commented Jan 6, 2023

ti-chi-bot commented Jan 6, 2023

add spiller #6568

add spiller #6568

Conversation

windtalker commented Jan 3, 2023

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Jan 3, 2023 • edited Loading

windtalker commented Jan 5, 2023

SeaRise left a comment

Choose a reason for hiding this comment

SeaRise Jan 5, 2023

Choose a reason for hiding this comment

windtalker Jan 6, 2023

Choose a reason for hiding this comment

SeaRise Jan 5, 2023

Choose a reason for hiding this comment

windtalker Jan 6, 2023

Choose a reason for hiding this comment

windtalker commented Jan 6, 2023

SeaRise left a comment

Choose a reason for hiding this comment

SeaRise Jan 6, 2023

Choose a reason for hiding this comment

windtalker Jan 6, 2023

Choose a reason for hiding this comment

mengxin9014 left a comment

Choose a reason for hiding this comment

mengxin9014 commented Jan 6, 2023

windtalker commented Jan 6, 2023

ti-chi-bot commented Jan 6, 2023

ti-chi-bot commented Jan 6, 2023

ti-chi-bot commented Jan 6, 2023

ti-chi-bot commented Jan 3, 2023 •

edited

Loading