-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
ui: use cluster setting from redux on schema insights #109047
Conversation
08b002a
to
4ff9e91
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.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @koorosh and @maryliag)
pkg/ui/workspaces/cluster-ui/src/api/databaseDetailsApi.ts
line 396 at r1 (raw file):
const getDatabaseIndexUsageStats: DatabaseDetailsQuery<IndexUsageStatistic> = { createStmt: (dbName: string, csIndexUnusedDuration: string) => { csIndexUnusedDuration = csIndexUnusedDuration ?? "168h";
Can the 168h be a shard constant somewhere to avoid future bugs if the value is changed?
Should there be a console log or something to hint that it's using the hard coded default instead of the clsuter setting? |
Part Of cockroachdb#108373 Use the value of the cluster setting `sql.index_recommendation.drop_unused_duration` from redux, instead of adding as part of the select. With this change, now users with VIEWACTIVITY or VIEWACTIVITYREDACTED can see index recommendations on the console, without the need the view cluster settings permission. This commit changes the Schema Insights Api. Release note (ui change): Users without `VIEWCLUSTERSETTINGS` permission but with `VIEWACTIVITY` or `VIEWACTIVITYREDACTED` can now see index recommendations.
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.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @j82w and @koorosh)
pkg/ui/workspaces/cluster-ui/src/api/databaseDetailsApi.ts
line 396 at r1 (raw file):
Previously, j82w (Jake) wrote…
Can the 168h be a shard constant somewhere to avoid future bugs if the value is changed?
Good idea! Done
pkg/ui/workspaces/cluster-ui/src/api/schemaInsightsApi.ts
line 150 at r1 (raw file):
Previously, j82w (Jake) wrote…
Should there be a console log or something to hint that it's using the hard coded default instead of the clsuter setting?
On the UI we are already showing the value that was used, so I don't think there are more details that need to be added.
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.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @koorosh)
bors r+ |
Build succeeded: |
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from f628393 to blathers/backport-release-23.1-109047: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 23.1.x failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
Part Of #108373
Use the value of the cluster setting
sql.index_recommendation.drop_unused_duration
from redux, instead of adding as part of the select.With this change, now users with VIEWACTIVITY or
VIEWACTIVITYREDACTED can see index recommendations on the console, without the need the view cluster settings permission.
This commit changes the Schema Insights Api.
https://www.loom.com/share/6b9ef154c9c44157a45e1e66b1fbc890
Release note (ui change): Users without
VIEWCLUSTERSETTINGS
permission but withVIEWACTIVITY
orVIEWACTIVITYREDACTED
can now see index recommendations.