Refine `Spiller` to support append write by windtalker · Pull Request #6862 · pingcap/tiflash

windtalker · 2023-02-20T08:15:27Z

What problem does this PR solve?

Issue Number: ref #6528

Problem Summary:

What is changed and how it works?

Add two config max_spilled_rows_per_file and max_spilled_bytes_per_file
Rename max_spilled_size_per_spill to max_cached_data_bytes_in_spiller and adjust the default value of it.
Support append write in spiller
Add append_dummy_read_stream to Spiller::restoreBlocks, if append_dummy_read_stream is true, Spiller::restoreBlocks will return at least max_stream_size streams by appending NullBlockInputStream
Add Block::estimateByteSizeForSpill to estimate the uncompressed bytes when the block is spilled.

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-02-20T08:15:29Z

[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.

Details

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

mengxin9014 · 2023-02-20T09:57:35Z

dbms/src/Core/SpillHandler.h

@@ -55,8 +56,10 @@ class SpillHandler
    size_t partition_id;
    Int64 current_spilled_file_index;


need initialize current_spilled_file_index and current_append_write in constructor, others LGTM.

They are inited in setUpNextSpilledFile, which is called in the constructor.

yibin87 · 2023-02-21T01:16:37Z

dbms/src/Columns/IColumn.h

    /// Size of column data in memory (may be approximate) - for profiling. Zero, if could not be determined.
    virtual size_t byteSize() const = 0;

+    virtual size_t estimateByteSizeForSpill() const { return byteSize(); }


Add a gtest for it?

dbms/src/Columns/ColumnAggregateFunction.cpp

yibin87 · 2023-02-21T01:36:37Z

dbms/src/Core/SpillHandler.cpp

@@ -63,24 +76,37 @@ void SpillHandler::spillBlocks(const Blocks & blocks)
    try


Better add random fail point here to check.

yibin87 · 2023-02-21T01:38:19Z

dbms/src/Core/SpillHandler.cpp

        LOG_INFO(spiller->logger, "Spilled {} rows from {} blocks into temporary file, time cost: {:.3f} sec.", total_rows, block_size, cost);
        RUNTIME_CHECK_MSG(current_spilled_file_index >= 0, "{}: spill after the spill handler is finished.", spiller->config.spill_id);
-        RUNTIME_CHECK_MSG(spiller->spill_finished == false, "{}: spill after the spiller is finished.", spiller->config.spill_id);
+        RUNTIME_CHECK_MSG(spiller->isSpillFinished() == false, "{}: spill after the spiller is finished.", spiller->config.spill_id);


why check finished at the end of the work?

Because spillBlocks not hold the lock, and it is not allowed to spill blocks after the spiller is finished, so add the check here to eary exit if error happens.

dbms/src/Core/Spiller.cpp

Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

dbms/src/Core/Spiller.cpp

dbms/src/Core/SpillHandler.cpp

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

SeaRise

Others LGTM

dbms/src/Core/SpillHandler.cpp

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

dbms/src/Core/SpillHandler.cpp

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

SeaRise

lgtm

windtalker · 2023-02-22T02:52:56Z

/merge

ti-chi-bot · 2023-02-22T02:52:57Z

@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.

Details

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-02-22T02:52:59Z

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

Details

Commit hash: 8cdc088

windtalker · 2023-02-22T03:17:33Z

/run-unit-test

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

windtalker · 2023-02-22T03:52:15Z

/merge

ti-chi-bot · 2023-02-22T03:52:16Z

@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.

Details

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-02-22T03:52:17Z

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

Details

Commit hash: 0692b9c

windtalker · 2023-02-22T03:57:34Z

/rebuild

windtalker · 2023-02-22T03:57:45Z

/run-integration-test

windtalker · 2023-02-22T03:59:10Z

/run-integration-test

windtalker · 2023-02-22T04:41:38Z

/run-unit-test

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

windtalker · 2023-02-22T05:37:37Z

/merge

ti-chi-bot · 2023-02-22T05:37:38Z

@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.

Details

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-02-22T05:37:40Z

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

Details

Commit hash: 8c28e99

* Refine `Spiller` to support append write (pingcap#6862) ref pingcap#6528 * fix build Signed-off-by: xufei <xufei@pingcap.com> * fix Signed-off-by: xufei <xufei@pingcap.com> * fix error Signed-off-by: xufei <xufei@pingcap.com> --------- Signed-off-by: xufei <xufei@pingcap.com>

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 Feb 20, 2023

mengxin9014 reviewed Feb 20, 2023

View reviewed changes

mengxin9014 approved these changes Feb 20, 2023

View reviewed changes

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

yibin87 reviewed Feb 21, 2023

View reviewed changes

SeaRise reviewed Feb 21, 2023

View reviewed changes

dbms/src/Columns/ColumnAggregateFunction.cpp Show resolved Hide resolved

yibin87 reviewed Feb 21, 2023

View reviewed changes

SeaRise reviewed Feb 21, 2023

View reviewed changes

dbms/src/Core/Spiller.cpp Outdated Show resolved Hide resolved

windtalker force-pushed the enhance_spiller branch from 4b596bb to dd017f8 Compare February 21, 2023 09:17

windtalker requested review from SeaRise and yibin87 February 21, 2023 10:14

windtalker added 8 commits February 22, 2023 07:46

enhance spiller

3cbd72e

Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

refine

5030bf9

Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

refine

1945f06

Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

add more ut

c190301

Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

add more tests

3cffb25

Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

address comments

baedbf6

Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

fix

97d95ae

Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

fix

851ff60

Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

windtalker force-pushed the enhance_spiller branch from dd017f8 to 851ff60 Compare February 21, 2023 23:50

SeaRise reviewed Feb 22, 2023

View reviewed changes

address comments

e6d974b

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

SeaRise reviewed Feb 22, 2023

View reviewed changes

dbms/src/Core/SpillHandler.cpp Outdated Show resolved Hide resolved

fix bug

b994971

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

SeaRise reviewed Feb 22, 2023

View reviewed changes

dbms/src/Core/SpillHandler.cpp Outdated Show resolved Hide resolved

SeaRise reviewed Feb 22, 2023

View reviewed changes

dbms/src/Core/SpillHandler.cpp Show resolved Hide resolved

windtalker added 2 commits February 22, 2023 10:32

address comments

612285b

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

refine

0aa58ba

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

remove todo

8cdc088

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

SeaRise approved these changes Feb 22, 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 Feb 22, 2023

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

update ut

0692b9c

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

ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Feb 22, 2023

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

fix ut

8c28e99

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

ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Feb 22, 2023

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

ti-chi-bot merged commit 94425db into pingcap:master Feb 22, 2023

windtalker deleted the enhance_spiller branch February 22, 2023 06:33

windtalker mentioned this pull request Feb 28, 2023

fix potential deadlock in spiller #6915

Merged

12 tasks

		@@ -55,8 +56,10 @@ class SpillHandler
		size_t partition_id;
		Int64 current_spilled_file_index;

		@@ -63,24 +76,37 @@ void SpillHandler::spillBlocks(const Blocks & blocks)
		try

Conversation

windtalker commented Feb 20, 2023

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

Uh oh!

ti-chi-bot commented Feb 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SeaRise left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SeaRise left a comment

Choose a reason for hiding this comment

Uh oh!

windtalker commented Feb 22, 2023

Uh oh!

ti-chi-bot commented Feb 22, 2023

Uh oh!

ti-chi-bot commented Feb 22, 2023

Uh oh!

windtalker commented Feb 22, 2023

Uh oh!

windtalker commented Feb 22, 2023

Uh oh!

ti-chi-bot commented Feb 22, 2023

Uh oh!

ti-chi-bot commented Feb 22, 2023

Uh oh!

windtalker commented Feb 22, 2023

Uh oh!

windtalker commented Feb 22, 2023

Uh oh!

windtalker commented Feb 22, 2023

Uh oh!

windtalker commented Feb 22, 2023

Uh oh!

windtalker commented Feb 22, 2023

Uh oh!

ti-chi-bot commented Feb 22, 2023

Uh oh!

ti-chi-bot commented Feb 22, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ti-chi-bot commented Feb 20, 2023 •

edited

Loading