-
Notifications
You must be signed in to change notification settings - Fork 454
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
Create historical versions of source statistics built in views #27135
Conversation
b975634
to
41371dc
Compare
Due to an issue with latency and high CPU usage, we create a non-historical index + view on top of mz_source_statistics such that Console's source lists efficiently query on it. The latency and high CPU usage is related to us retaining history, causing the number of updates in the index to be large.
41371dc
to
d254907
Compare
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.
I'm not sure how this works, but does renaming those indexes mean we will loose all the existing historical data?
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.
This will need signoff from the epd-mz-introspection-council due to a new index.
src/pgrepr-consts/src/oid.rs
Outdated
@@ -674,3 +674,5 @@ pub const INDEX_MZ_KAFKA_SOURCES_IND_OID: u32 = 16950; | |||
pub const INDEX_MZ_WEBHOOK_SOURCES_IND_OID: u32 = 16951; | |||
pub const TABLE_MZ_HISTORY_RETENTION_STRATEGIES_OID: u32 = 16952; | |||
pub const SOURCE_MZ_MATERIALIZED_VIEW_REFRESHES_OID: u32 = 16953; | |||
pub const VIEW_MZ_SOURCE_STATISTICS_OID: u32 = 16954; |
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.
If this name is staying, it should keep its old oid. Give VIEW_MZ_SOURCE_STATISTICS_WITH_HISTORY_OID and the other new thing new oids instead.
@RobinClowers My first guess would be no given the historical view derives from |
You can run To fix the sqllogictests run The testdrive test will need manual rewriting, but its diff is printed in the test failure output so should be applied by hand. I'm not going to push up a fix for these because I think changing the oids as requested will result in more changes, but these may be enough to unblock you. |
LGTM from mz_introspection council. The new index has only a very small number of records, so the memory impact should be minimal. |
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.
Good chance that the autogenerated/docs tests will fail due to the new things not being documented. ./ci/test/lint-docs-catalog.sh --rewrite
may help there, and probably need to add docs.
d0f9731
to
c418769
Compare
such that existing schema doesn't need to change
c418769
to
4140341
Compare
Due to an issue with latency and high CPU usage, we create a non-historical index + view on top of mz_source_statistics such that Console's source lists efficiently query on it. The latency and high CPU usage is related to us retaining history, causing the number of updates in the index to be large.
From 5 cases, there seems to be a stark improvement with these new indexes:
Motivation
Tips for reviewer
Checklist
$T ⇔ Proto$T
mapping (possibly in a backwards-incompatible way), then it is tagged with aT-proto
label.