-
Notifications
You must be signed in to change notification settings - Fork 610
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
Change TEvLocal actorsystem metrics #6355
Conversation
⚪ |
⚪ |
⚪ |
9a1d4d5
to
6b9fe41
Compare
⚪
🟡
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
⚪
🟡
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
ydb/core/protos/local.proto
Outdated
@@ -53,6 +63,7 @@ message TEvStatus { | |||
optional uint64 AvailableWeight = 5; | |||
optional NKikimrTabletBase.TMetrics ResourceMaximum = 8; | |||
optional uint64 StartTime = 7; | |||
optional TActorSystemInfo ActorSystemInfo = 9; |
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.
TEvStatus is sent when Local first registers in Hive, and when the tenant is altered. If we want this info to be sent regularly, I would put in TabletMetrics message.
ydb/core/mind/local.cpp
Outdated
@@ -111,6 +111,9 @@ class TLocalNodeRegistrar : public TActorBootstrapped<TLocalNodeRegistrar> { | |||
ui64 MemLimit = 0; | |||
double NodeUsage = 0; | |||
|
|||
TInstant LastUpdate; |
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.
The name is confusing, last update of what? Also If we change it to use TabletMetrics message, there already is a SendTabletMetricsTime member
ydb/core/mind/local.cpp
Outdated
double cores = 0; | ||
for (ui8 poolId = 0; poolId < poolStates.size(); ++poolId) { | ||
auto &poolState = poolStates[poolId]; | ||
if (poolId != AppData()->IOPoolId) { |
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.
We already have a setting PoolsToMonitorForUsage. I believe it should be used here. But it also excludes Batch pool by default - is there a reason you're including it here?
ydb/core/protos/local.proto
Outdated
@@ -32,6 +32,16 @@ message TLocalConfig { | |||
repeated NKikimrSchemeOp.TResourceProfile ResourceProfiles = 1; | |||
} | |||
|
|||
message TActorSystemInfo { |
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.
Currently we send a single number TotalNodeUsage, that is the maximum over pool usages (and mem usage).
What do we expect to gain from sending this detailed info? Just seems like we will simply do the calculate usage + take maximum logic on Hive side instead.
6b9fe41
to
98f4679
Compare
⚪
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
⚪
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
Changelog entry
...
Changelog category
Additional information
...