-
Notifications
You must be signed in to change notification settings - Fork 6.9k
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
Add statement SYSTEM RELOAD ASYNCHRONOUS METRICS
#53710
Add statement SYSTEM RELOAD ASYNCHRONOUS METRICS
#53710
Conversation
This is an automated comment for commit 8fc918b with description of existing statuses. It's updated for the latest CI running ❌ Click here to open a full report in a separate page Successful checks
|
Ok. Caveats:
|
Not sure I understand this part, but |
SYSTEM RELOAD ASYNCHRONOUS METRICS
I refreshed this PR after a long time. Alexey's points are addressed. @antonio2368 maybe you like to take a look? (I guess this PR appears so far in the open PR list on GitHub that nobody will assign 😄) |
f54d896
to
0929b09
Compare
@@ -0,0 +1 @@ | |||
SYSTEM RELOAD ASYNCHRONOUS METRICS; |
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.
you can make a much better test
set asynchronous_metrics_update_period_s
to a really large value
and then pick a metric, e.g. Uptime
or NumberOfTables
to check in system.asynchronous_metrics
it's updated after this call
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.
Done, thanks.
getContext()->checkAccess(AccessType::SYSTEM_RELOAD_ASYNCHRONOUS_METRICS); | ||
auto * asynchronous_metrics = system_context->getAsynchronousMetrics(); | ||
if (asynchronous_metrics) | ||
asynchronous_metrics->update(std::chrono::system_clock::now()); |
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.
now I see what Alexey was saying
In server metrics there is updateHeavyMetricsIfNeeded
which has a different update interval.
They won't be updated with this call unless the interval has passed since last update.
I think you need to add a new argument bool force
to update
and updateImpl
which will ignore such checks.
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.
Right, thanks. Updated.
0929b09
to
90a0ea3
Compare
There were Tsan issues and weird results (probably due to races) in previous commits. |
…ync-metrics Minor follow-up to #53710
This PR was born out of an annoyance with integration tests where it was necessary to prepend
SELECT * FROM system.asynchronous_metrics
withsleep(sth > asynchronous_metrics_update_interval)
to make sure the system tables are up-to-date.Commits:
SYSTEM RELOAD ASYNCHRONOUS METRICS
90a0ea3Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Added statement
SYSTEM RELOAD ASYNCHRONOUS METRICS
which updates the asynchronous metrics. Mostly useful for testing and development.