-
Notifications
You must be signed in to change notification settings - Fork 409
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
KVStore: Record table-wise memory usage and warn when it goes beyond the threshold #9835
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Calvin Neo <[email protected]>
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: Calvin Neo <[email protected]>
Signed-off-by: Calvin Neo <[email protected]>
Signed-off-by: Calvin Neo <[email protected]>
Signed-off-by: Calvin Neo <[email protected]>
Signed-off-by: Calvin Neo <[email protected]>
Signed-off-by: Calvin Neo <[email protected]>
Signed-off-by: Calvin Neo <[email protected]>
Signed-off-by: Calvin Neo <[email protected]>
Signed-off-by: Calvin Neo <[email protected]>
Signed-off-by: Calvin Neo <[email protected]>
Signed-off-by: Calvin Neo <[email protected]>
Signed-off-by: Calvin Neo <[email protected]>
Signed-off-by: Calvin Neo <[email protected]>
Signed-off-by: Calvin Neo <[email protected]>
Signed-off-by: Calvin Neo <[email protected]>
Signed-off-by: Calvin Neo <[email protected]>
Signed-off-by: Calvin Neo <[email protected]>
Signed-off-by: Calvin Neo <[email protected]>
Signed-off-by: Calvin Neo <[email protected]>
Co-authored-by: JaySon <[email protected]>
Co-authored-by: JaySon <[email protected]>
Signed-off-by: Calvin Neo <[email protected]>
RegionTable::InternalRegion & RegionTable::insertRegion(Table & table, const Region & region) | ||
{ | ||
const auto range = region.getRange(); | ||
return insertRegion(table, *range, region.id()); | ||
return insertRegion(table, *range, region); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make line 108 in RegionTable::getOrInsertRegion
as return insertRegion(table, *region.getRange(), region);
and remove this redundant function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
|
||
Type total() const { return payload + decoded; } | ||
|
||
size_t totalSize() const { return payload + decoded; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why we need two method for the same result? Maybe just adding explicit cast when want to assign the result to differnet type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
total()
needs no casting, but is not used very much,
totalSize()
needs casting, but is widely used in test, so it is we just write it here to eliminate some static_cast
ing
std::atomic_int64_t table_size; | ||
std::atomic_bool warned; | ||
}; | ||
using RegionTableCtx = std::shared_ptr<RegionTableCtxInner>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe rename to RegionTableCtxPtr
? Most alias of shared_ptr are xxxPtr
in TiFlash.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just keep those function name to make them shorter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just keep those function name to make them shorter?
I think its OK.
Co-authored-by: jinhelin <[email protected]>
Co-authored-by: JaySon <[email protected]>
Signed-off-by: Calvin Neo <[email protected]>
Signed-off-by: Calvin Neo <[email protected]>
Signed-off-by: Calvin Neo <[email protected]>
Signed-off-by: Calvin Neo <[email protected]>
@CalvinNeo: The following test failed, say
Full PR test history. Your PR dashboard. 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |
What problem does this PR solve?
Issue Number: ref #9722
Problem Summary:
RegionTableSize
which is shared by aTable
and all its relatedRegion
s. Once registered, a region will update its memory change to this struct, so the table would learn.maybeWarnMemoryLimitByTable
andresetWarnMemoryLimitByTable
, to log when the memory of a table goes beyond threshold.Both of the systems works under the path that:
RegionTableSize
.RegionTableSize
.Also:
Region
to reflect what they actually do.What is changed and how it works?
Check List
Tests
Side effects
Documentation
Release note