Storage: (Refactor) Avoid referencing DeltaMergeStore.h #5835
Conversation
Signed-off-by: Wish <[email protected]>
Signed-off-by: Wish <[email protected]>
Signed-off-by: Wish <[email protected]>
Signed-off-by: Wish <[email protected]>
Signed-off-by: Wish <[email protected]>
|
[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. Reviewer can indicate their review by submitting an approval review. |
ti-chi-bot
added
release-note-none
Signed-off-by: Wish <[email protected]>
ti-chi-bot
added
size/L
| @@ -14,29 +14,24 @@ | |||
|
|
|||
| #pragma once | |||
|
|
|||
| #include <Common/Logger.h> | |||
There was a problem hiding this comment.
Can we remove this header file？
There was a problem hiding this comment.
It should be a better practice to include everything you used in this header file as possible, even if it may be included transitively by other included header files. In this way, this header is less likely to break when other headers are refactored.
In this case, as StorageDeltaMerge has a field of type LoggerPtr, we could explicitly introduce it.
There was a problem hiding this comment.
We can use forward declaration here, using
namespace DB
{
class Logger;
using LoggerPtr = std::shared_ptr<Logger>;
}
instead of including the <Common/Logger.h>.
Here I think both are OK since Common/Logger.h itself is a clean header file that doesn't bring many other header files.
|
/run-all-tests |
Coverage for changed filesCoverage summaryfull coverage report (for internal network access only) |
| F(type_delta_merge_bg_gc, {"type", "delta_merge_bg_gc"}), \ | ||
| F(type_delta_merge_fg, {"type", "delta_merge_fg"}), \ | ||
| F(type_delta_merge_manual, {"type", "delta_merge_manual"}), \ |
There was a problem hiding this comment.
Does grafana need to adjust?
There was a problem hiding this comment.
In Grafana expressions are using "delta_merge|seg" as matchers, so there is no need to adjust.
ti-chi-bot
added
status/LGT2
|
/merge |
|
@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. 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. |
|
This pull request has been accepted and is ready to merge. Commit hash: d85449e
|
ti-chi-bot
added
the
status/can-merge
Coverage for changed filesCoverage summaryfull coverage report (for internal network access only) |
What problem does this PR solve?
Issue Number: ref #5831
DeltaMerge/DeltaMergeStore.his imported inStorages/StorageDeltaMerge.h, which makes the pimpl useless -- When changing private member functions inDeltaMerge/DeltaMergeStore.h, a lot of external functions need to be recompiled.What is changed and how it works?
TaskRunThreadtoMergeDeltaReason, which makes more sense.TaskRunThreadorMergeDeltaReasonin DeltaMerge public APImergeDeltaBySegment, as run thread or merge reason are purely internal.RowKeyValueso that the import of internal DeltaMerge headers can be avoided.Check List
Tests
Side effects
Documentation
Release note