Integrate some functions in KVStore and rename them to what they actually do #7402

CalvinNeo · 2023-04-27T08:32:38Z

What problem does this PR solve?

Issue Number: ref #7256

Problem Summary:

Some methods are delegators of writeBlockByRegion which transforms row-format data into columnar-format data, and write them to cache of DeltaMergeStore.
There methods don't always do a flush. Actually, they ALWAYS NOT DO a flush, the only exception is when they are called from tryFlushRegions, and that is purely a scene of debugger.

flushRegion
tryPersist this is some un-locked version of persistRegion, we change its name to tryPersistRegion, and make it coworks with persistRegion.
tryFlushRegion
tryFlushRegions

So these names are quite misleading, I think we have to change them to what they actually do.

What is changed and how it works?

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

Signed-off-by: CalvinNeo <[email protected]>

ti-chi-bot · 2023-04-27T08:32:40Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

JaySon-Huang
lidezhu

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.

Signed-off-by: CalvinNeo <[email protected]>

dbms/src/Debug/MockRaftStoreProxy.cpp

dbms/src/Storages/Transaction/Region.cpp

Signed-off-by: CalvinNeo <[email protected]>

CalvinNeo · 2023-04-27T09:46:20Z

/run-all-tests

lidezhu · 2023-04-27T10:20:52Z

dbms/src/Debug/dbgFuncRegion.cpp

@@ -101,7 +101,7 @@ void dbgFuncPutRegion(Context & context, const ASTs & args, DBGInvoker::Printer
 void dbgFuncTryFlush(Context & context, const ASTs &, DBGInvoker::Printer output)
 {
    TMTContext & tmt = context.getTMTContext();
-    tmt.getRegionTable().tryFlushRegions();


I think function name should describe what job it intend to do not how to do the job. So I prefer the previous one.
And this function is actually deprecated, so I think there is no need to change its name.

I don't know what job dbgFuncTryFlush intends to do. However, if dbgFuncTryFlush is deprecated, we can remove dbgFuncTryFlush and tryFlushRegions

Signed-off-by: CalvinNeo <[email protected]>

CalvinNeo · 2023-04-28T02:19:35Z

/run-all-tests

CalvinNeo · 2023-04-28T03:17:40Z

/run-integration-test

lidezhu

LGTM

JaySon-Huang · 2023-04-28T07:08:19Z

dbms/src/Storages/Transaction/RegionTable.h

-    RegionDataReadInfoList flushRegion(const RegionPtrWithBlock & region, bool try_persist) const;
+    // Try write the committed kvs into cache of columnar DeltaMergeStore.
+    // Flush the cache if try_persist is set to true.
+    // The original name for this method is flushRegion.


Suggested change

// The original name for this method is flushRegion.

I think we don't need to keep this message... it doesn't help

JaySon-Huang · 2023-04-28T07:08:26Z

dbms/src/Storages/Transaction/RegionTable.h

+
+    // Protects writeBlockByRegionAndFlush and ensures it's executed by only one thread at the smae time.
+    // Only one thread can do this at the same time.
+    // The original name for this function is tryFlushRegion.


Suggested change

// The original name for this function is tryFlushRegion.

dbms/src/Storages/Transaction/KVStore.cpp

Signed-off-by: CalvinNeo <[email protected]>

JaySon-Huang

LGTM

CalvinNeo · 2023-04-28T08:51:17Z

/merge

ti-chi-bot · 2023-04-28T08:51:19Z

@CalvinNeo: 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-04-28T08:51:20Z

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

Commit hash: 292f16d

ti-chi-bot · 2023-04-28T09:16:04Z

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

…ally do (pingcap#7402) ref pingcap#7256

CalvinNeo added 2 commits April 27, 2023 16:20

f

ce93bb6

Signed-off-by: CalvinNeo <[email protected]>

zzz

4eb7f47

Signed-off-by: CalvinNeo <[email protected]>

ti-chi-bot bot added do-not-merge/needs-linked-issue release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 27, 2023

z

aaf4c29

Signed-off-by: CalvinNeo <[email protected]>

CalvinNeo requested review from JaySon-Huang, lidezhu and breezewish April 27, 2023 09:32

ti-chi-bot bot removed the do-not-merge/needs-linked-issue label Apr 27, 2023

CalvinNeo changed the title ~~Integrate some functions in KVStore, and make them more readable~~ Integrate some functions in KVStore, and rename them to what they actually do Apr 27, 2023

CalvinNeo changed the title ~~Integrate some functions in KVStore, and rename them to what they actually do~~ Integrate some functions in KVStore and rename them to what they actually do Apr 27, 2023

CalvinNeo commented Apr 27, 2023

View reviewed changes

dbms/src/Debug/MockRaftStoreProxy.cpp Outdated Show resolved Hide resolved

CalvinNeo commented Apr 27, 2023

View reviewed changes

dbms/src/Storages/Transaction/Region.cpp Outdated Show resolved Hide resolved

fix tyoe

61b28e1

Signed-off-by: CalvinNeo <[email protected]>

lidezhu reviewed Apr 27, 2023

View reviewed changes

CalvinNeo added 2 commits April 27, 2023 18:29

f

2f8ecd8

Signed-off-by: CalvinNeo <[email protected]>

gggg

49fdab8

Signed-off-by: CalvinNeo <[email protected]>

CalvinNeo requested a review from lidezhu April 28, 2023 02:12

lidezhu approved these changes Apr 28, 2023

View reviewed changes

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

JaySon-Huang reviewed Apr 28, 2023

View reviewed changes

ad

7947cda

Signed-off-by: CalvinNeo <[email protected]>

JaySon-Huang approved these changes Apr 28, 2023

View reviewed changes

ti-chi-bot bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Apr 28, 2023

Merge branch 'master' into improv-kvstore-names

292f16d

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

Merge branch 'master' into improv-kvstore-names

a25b21b

ti-chi-bot bot merged commit c154ad2 into pingcap:master Apr 28, 2023

CalvinNeo added a commit to CalvinNeo/tiflash that referenced this pull request Jun 30, 2023

Integrate some functions in KVStore and rename them to what they actu…

df781c5

…ally do (pingcap#7402) ref pingcap#7256

CalvinNeo added a commit to CalvinNeo/tiflash that referenced this pull request Jul 3, 2023

Integrate some functions in KVStore and rename them to what they actu…

56eafc8

…ally do (pingcap#7402) ref pingcap#7256

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate some functions in KVStore and rename them to what they actually do #7402

Integrate some functions in KVStore and rename them to what they actually do #7402

CalvinNeo commented Apr 27, 2023 •

edited

Loading

ti-chi-bot bot commented Apr 27, 2023 •

edited

Loading

CalvinNeo commented Apr 27, 2023

lidezhu Apr 27, 2023

CalvinNeo Apr 27, 2023

CalvinNeo commented Apr 28, 2023

CalvinNeo commented Apr 28, 2023

lidezhu left a comment

JaySon-Huang Apr 28, 2023

CalvinNeo Apr 28, 2023

JaySon-Huang Apr 28, 2023

JaySon-Huang Apr 28, 2023

JaySon-Huang left a comment

CalvinNeo commented Apr 28, 2023

ti-chi-bot bot commented Apr 28, 2023

ti-chi-bot bot commented Apr 28, 2023

ti-chi-bot bot commented Apr 28, 2023

Integrate some functions in KVStore and rename them to what they actually do #7402

Integrate some functions in KVStore and rename them to what they actually do #7402

Conversation

CalvinNeo commented Apr 27, 2023 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot bot commented Apr 27, 2023 • edited Loading

CalvinNeo commented Apr 27, 2023

lidezhu Apr 27, 2023

Choose a reason for hiding this comment

CalvinNeo Apr 27, 2023

Choose a reason for hiding this comment

CalvinNeo commented Apr 28, 2023

CalvinNeo commented Apr 28, 2023

lidezhu left a comment

Choose a reason for hiding this comment

JaySon-Huang Apr 28, 2023

Choose a reason for hiding this comment

CalvinNeo Apr 28, 2023

Choose a reason for hiding this comment

JaySon-Huang Apr 28, 2023

Choose a reason for hiding this comment

JaySon-Huang Apr 28, 2023

Choose a reason for hiding this comment

JaySon-Huang left a comment

Choose a reason for hiding this comment

CalvinNeo commented Apr 28, 2023

ti-chi-bot bot commented Apr 28, 2023

ti-chi-bot bot commented Apr 28, 2023

ti-chi-bot bot commented Apr 28, 2023

CalvinNeo commented Apr 27, 2023 •

edited

Loading

ti-chi-bot bot commented Apr 27, 2023 •

edited

Loading