[CRDB-13198] ui, server: update cluster settings to include last update time #78034
Conversation
|
This change is |
Santamaura
force-pushed
the
update-cluster-settings
branch
3 times, most recently
from
cefe5b9 to
85d2d19
Compare
pkg/server/admin.go
Outdated
| if it, err := s.server.sqlServer.internalExecutor.QueryIteratorEx( | ||
| ctx, "read-setting", nil, /* txn */ | ||
| sessiondata.InternalExecutorOverride{User: security.RootUserName()}, | ||
| "SELECT * FROM system.settings", |
There was a problem hiding this comment.
please specify the column names explicitly. This ensures the code won't break if/when we add more columns to the table or reorder the columns.
pkg/server/admin_test.go
Outdated
| if v.LastUpdated != nil { | ||
| db := sqlutils.MakeSQLRunner(conn) | ||
| q := makeSQLQuery() | ||
| q.Append(`SELECT * FROM system.settings WHERE name=$`, k) |
Santamaura
force-pushed
the
update-cluster-settings
branch
from
85d2d19 to
7e54f67
Compare
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @knz, @koorosh, and @zachlite)
pkg/server/admin.go, line 1760 at r1 (raw file):
Previously, knz (kena) wrote…
please specify the column names explicitly. This ensures the code won't break if/when we add more columns to the table or reorder the columns.
Done.
pkg/server/admin_test.go, line 1126 at r1 (raw file):
Previously, knz (kena) wrote…
ditto.
Done.
Santamaura
changed the title
There was a problem hiding this comment.
Reviewable status:
-- commits, line 16 at r2:
nit: fix release note
pkg/ui/workspaces/db-console/src/views/reports/containers/settings/index.tsx, line 112 at r2 (raw file):
: "No overrides", sort: (setting: IterableSetting) => util.TimestampToMoment(setting.last_updated).valueOf(),
nit: can we precompute the TimestampToMoment so it doesn't happen twice in here?
There was a problem hiding this comment.
Reviewed 3 of 5 files at r1, 1 of 2 files at r2.
Reviewable status:
pkg/server/admin.go, line 1760 at r2 (raw file):
ctx, "read-setting", nil, /* txn */ sessiondata.InternalExecutorOverride{User: security.RootUserName()}, `SELECT name, "lastUpdated" FROM system.settings`,
User can request specific settings with req.Keys. Can you extend query with condition to retrieve specific settings in this case?
Resolves cockroachdb#76626 Previously it was difficult to determine from the cluster settings page which settings have been altered. This patch updates the cluster settings endpoint to include a last updated field, indicating when the setting was last altered, which allows the settings page to include that column in the table and is pre-sorted by this value. Release justification: Fairly minor changes for QoL upgrade Release note: None
Santamaura
force-pushed
the
update-cluster-settings
branch
from
7e54f67 to
2ded655
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/server/admin.go, line 1760 at r2 (raw file):
Previously, koorosh (Andrii Vorobiov) wrote…
User can request specific settings with
req.Keys. Can you extend query with condition to retrieve specific settings in this case?
Hm, it was pointed out to me that in the process of formatting req.Keys for the sql query that using the user input may be a risk for sql injection. I'll leave as is for now but if you know there is a way that is security safe to make this change lmk!
pkg/ui/workspaces/db-console/src/views/reports/containers/settings/index.tsx, line 112 at r2 (raw file):
Previously, dhartunian (David Hartunian) wrote…
nit: can we precompute the
TimestampToMomentso it doesn't happen twice in here?
Done.
There was a problem hiding this comment.
Reviewed 1 of 2 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status:
|
bors r+ |
|
Build succeeded: |
|
Heads up @Santamaura this change isn't available in v22.1.0 - this might have missed the original release train for 22.1 |
Resolves #76626
Previously it was difficult to determine from the cluster
settings page which settings have been altered.
This patch updates the cluster settings endpoint to include a
last updated field, indicating when the setting was last altered,
which allows the settings page to include that column in the
table and is pre-sorted by this value.
Release note: None
Release justification: Fairly minor changes for QoL upgrade