feat: support the new compact RPC request#4885
Merged
ti-chi-bot merged 24 commits intomasterfrom May 18, 2022
Merged
Conversation
Member
|
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. DetailsReviewer can indicate their review by submitting an approval review. |
breezewish
commented
May 13, 2022
Signed-off-by: Wish <breezewish@outlook.com>
Signed-off-by: Wish <breezewish@outlook.com>
Signed-off-by: Wish <breezewish@outlook.com>
Signed-off-by: Wish <breezewish@outlook.com>
Signed-off-by: Wish <breezewish@outlook.com>
Member
Author
|
The kvproto change has been merged into the PR. |
flowbehappy
reviewed
May 16, 2022
| auto dm_context = newDMContext(context, context.getSettingsRef(), /*tracing_id*/ "mergeDeltaBySegment"); | ||
| if (start_key.is_common_handle != dm_context->is_common_handle) | ||
| { | ||
| return std::nullopt; |
Contributor
There was a problem hiding this comment.
Simply throw an exception. It is unexpected status.
Member
Author
There was a problem hiding this comment.
The start_key comes from the RPC request. From the TiFlash perspective maybe better to think all RPC callers to be a bad guy in order to isolate component errors. Otherwise a mistake in TiDB may cause TiFlash to crash.
Member
Author
There was a problem hiding this comment.
Or may be better to leave this check in the ManualCompactManager? We could keep a property that all calls to DeltaMergeStore should be checked before calling.
Contributor
There was a problem hiding this comment.
IMO, it is better to rise some serious thing to notify the unexpected status, as it implies bugs usually. How about returning errors to TiDB when there are invalid params?
Contributor
There was a problem hiding this comment.
BTW throw exception here should not crash TiFlash.
Member
Author
There was a problem hiding this comment.
I have moved this check to the TiFlash service side. Please take a look again, thanks!
Also added a test in the DM to ensure that, when common handle is given for a int handle table, there will be an exception thrown.
flowbehappy
reviewed
May 16, 2022
|
|
||
| std::optional<DM::RowKeyRange> DeltaMergeStore::mergeDeltaBySegment(const Context & context, const RowKeyValue & start_key) | ||
| { | ||
| auto dm_context = newDMContext(context, context.getSettingsRef(), /*tracing_id*/ "mergeDeltaBySegment"); |
Contributor
There was a problem hiding this comment.
I think we should use updateGCSafePoint before creating the DMContext
Contributor
There was a problem hiding this comment.
And add the value of latest_gc_safepoint to the "tracing_id"
Member
Author
There was a problem hiding this comment.
Updated.
Just curious, if we don't update the safepoint here, what will happen in worst case?
Member
Author
There was a problem hiding this comment.
BTW updating GC safepoint here means we will send 1 PD request for every segment. Is this fine? (Do we need something like throttle?)
Member
Author
There was a problem hiding this comment.
Another solution is to let updateGCSafePoint to be public, so that it will only get called once per compact request. I'm not sure whether this might leak the encapsulation.
Contributor
There was a problem hiding this comment.
Just curious, if we don't update the safepoint here, what will happen in worst case?
When the origin segment doesn't have any data in delta, and the stable contains rows that are older than gc safepoint. Even after we apply "merge delta" on the segment, we take some resources to rewrite the stable data (but it is effectively the same as before), but we don't get any performance improvement.
Comment on lines
+366
to
+367
| M(SettingUInt64, manual_compact_max_concurrency, 10, "Max concurrent tasks. It should be larger than pool size.") \ | ||
| M(SettingUInt64, manual_compact_more_until_ms, 60000, "Continuously compact more segments until reaching specified elapsed time. If 0 is specified, only one segment will be compacted each round.") |
Contributor
There was a problem hiding this comment.
Seems these two configs are not shown in the "config-template.toml" file?
Member
Author
There was a problem hiding this comment.
Yes, they are advanced config that normal users should not modify.
lidezhu
reviewed
May 17, 2022
Contributor
|
/run-all-tests |
1 similar comment
Member
Author
|
/run-all-tests |
Member
Author
|
/run-all-tests |
Collaborator
Coverage for changed filesCoverage summaryfull coverage report (for internal network access only) |
Member
Author
|
/merge |
Member
|
@breezewish: It seems you want to merge this PR, I will help you trigger all the tests: /run-all-tests You only need to trigger If you have any questions about the PR merge process, please refer to pr process. DetailsInstructions 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. |
Member
|
This pull request has been accepted and is ready to merge. DetailsCommit hash: 2d907ec |
Collaborator
Coverage for changed filesCoverage summaryfull coverage report (for internal network access only) |
Member
Author
|
/merge |
Member
|
@breezewish: It seems you want to merge this PR, I will help you trigger all the tests: /run-all-tests You only need to trigger If you have any questions about the PR merge process, please refer to pr process. DetailsInstructions 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. |
Collaborator
Coverage for changed filesCoverage summaryfull coverage report (for internal network access only) |
Collaborator
Coverage for changed filesCoverage summaryfull coverage report (for internal network access only) |
Member
Author
|
/run-integration-test |
Collaborator
Coverage for changed filesCoverage summaryfull coverage report (for internal network access only) |
Collaborator
Coverage for changed filesCoverage summaryfull coverage report (for internal network access only) |
Member
|
@breezewish: 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. DetailsInstructions 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. |
Collaborator
Coverage for changed filesCoverage summaryfull coverage report (for internal network access only) |
This was referenced May 20, 2022
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: ref #4897
Problem Summary:
This PR adds the implementation of the Compact RPC. The Compact RPC is defined as:
Compact one or more data partials (Segments in DeltaTree) according to the specified Table ID, and returns the key for compacting more data partials.
See detailed RPC interface definition in tiflash: Add TiFlash compaction RPC and messages kvproto#918
The TiDB PR to utilize this RPC endpoint: pingcap/tidb#34741
What is changed and how it works?
New:
The impl of Compact RPC:
Flash/Management/ManualCompact.cpp|h.Public API to compact one segment in DeltaTree:
Storages/DeltaMerge/*: Mainlystd::optional<DM::RowKeyRange> mergeDeltaBySegment(const Context & context, const DM::RowKeyValue & start_key);Unit tests about the compact RPC and compact one segment: See
tests.Change:
tiflashErrorCodeToGrpcStatusCodefrom theCoprocessorHandlerto a new shared utility calledServiceUtils, so that Compact RPC can use it as well.Check List
Tests
Side effects
Documentation
Release note
None, as the user interface is in the TiDB side.