ui: databases page scales to handle large number of dbs #96431
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @j82w)
pkg/ui/workspaces/cluster-ui/src/api/databasesApi.ts line 60 at r1 (raw file):
if ( result.error && !result.error.message.endsWith("max result size exceeded")
What would you think about switching these checks around so we don't have to depend on the text of the error message? Perhaps getting back both result rows and an error means that we've been size-limited?
There was a problem hiding this comment.
Could you update the buildSQLApiDatabasesResponse function used in fakeApi.ts for tests in api.spec.ts. Now that we're only getting the database_name from SHOW DATABASES we don't need to mock the other columns (they aren't tested, but worth changing for accuracy).
Reviewable status:
There was a problem hiding this comment.
Also, should we consider removing the ability to sort by other columns (at least temporarily) if we know they can break the page for large table sets?
Imo, the only columns worth sorting on would be size/range count and it's not a big loss if users aren't using them much anyways. Worth having some discussion on this imo. It'd be nice to really get some idea on whether these sorting options are used much, @kevin-v-ngo do we have any data on this?
Reviewable status:
|
I'm fine with removing the sort capability if @kevin-v-ngo agrees. |
|
Previously, matthewtodd (Matthew Todd) wrote…
Done. |
Can you clarify what you mean by other columns? Are we removing sorting for all? If this page breaks, let's remove the capability. We should file a follow-up issue to track adding the behavior back in. Especially sorting by size. |
|
I'm assuming we already sort by default on size correct? FYI @dongniwang |
|
The default sort is by name. Sorting by name works because all the names are loaded in memory. It can handle about 2000 names depending on how big the names are before hitting the size limit. The table sorting will break for any column except the database name if there are more than like 50 dbs because it will get throttled. If you ask to sort by size then it requires doing 2k network calls to get the details of all dbs rather than just the details for the first page. It can probably handle more depending on the configuration. |
There was a problem hiding this comment.
Code
I wonder (in follow-on work) if it would be interesting / valuable to re-enable sorting below a certain number of databases, when we could do it efficiently enough. But I don't deeply understand if / how people use sorting today.
Reviewable status:
There was a problem hiding this comment.
Oops, published without approving! Also I think @THardy98 already said what I was thinking. 😄
Reviewable status:
|
I think the default is sort by name...but I'd imagine sort by size would be used by users on this page. Do we not have pagination for this page? What was the initial reasoning? |
There was a problem hiding this comment.
Just left some small nits. Really like this change. Some follow up work to batch these requests would be great.
Reviewed 2 of 4 files at r1, 1 of 2 files at r2, 1 of 2 files at r3, all commit messages.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/api/databasesApi.ts line 52 at r3 (raw file):
timeout, ).then(result => { // If request succeeded but query failed, throw error (caught by saga/cacheDataReducer).
This isn't entirely accurate anymore, we only throw an error when the request succeeds with empty results and throws an error.
pkg/ui/workspaces/cluster-ui/src/databasesPage/databasesPage.tsx line 303 at r3 (raw file):
let filteredDbs = this.props.databases; console.log("refresh: " + JSON.stringify(this.props.sortSetting));
Do we want to log on refresh? We can probably remove this.
pkg/ui/workspaces/cluster-ui/src/databasesPage/databasesPage.tsx line 307 at r3 (raw file):
// 40 dbs. If there is more than 40 dbs sort will be disabled. if (this.props.databases.length > disableTableSortSize) { let startIndex = 0;
We are reassigning this immediately below, we can initialize instead as:
let startIndex = this.state.pagination.pageSize * (this.state.pagination.current - 1);
pkg/ui/workspaces/cluster-ui/src/databasesPage/databasesPage.tsx line 315 at r3 (raw file):
} if (!filteredDbs || filteredDbs.length === 0) {
Do we need the !filteredDbs check if we assign it to all databases by default?
pkg/ui/workspaces/cluster-ui/src/databasesPage/databasesPage.tsx line 335 at r3 (raw file):
} filteredDbs.forEach((database, i, arr) => {
We don't seem to be using the i or arr params, can just be (database).
pkg/ui/workspaces/cluster-ui/src/sortedtable/sortedtable.tsx line 106 at r3 (raw file):
loading?: boolean; loadingLabel?: string; disableSortForLargeData: number;
Maybe could use a better name, disableSortForLargeData sounds like a boolean value. disableSortSizeLimit or something similar might be more intuitive.
pkg/ui/workspaces/cluster-ui/src/sortedtable/sortedtable.tsx line 231 at r3 (raw file):
if ( this.props.disableSortForLargeData &&
disableSortForLargeData isn't an optional parameter, are we checking for a non-zero value?
We're comparing data.length to it anyways in the conditional below so not sure if this check is necessary.
pkg/ui/workspaces/cluster-ui/src/sortedtable/sortedtable.tsx line 266 at r3 (raw file):
) => { const sort = !this.props.disableSortForLargeData ||
Ditto.
The databases page displays partial results instead of just showing an error message. Sorting is disabled if there are more than 2 pages of results which is currently configured to 40dbs. This still allows most user to use sort functionality, but prevents large customers from breaking when it would need to do a network call per a database. The database details are now loaded on demand for the first page only. Previously a network call was done for all databases which would result in 2k network calls. It now only loads the page of details the user is looking at. part of: #94332 Release note: none
There was a problem hiding this comment.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/api/databasesApi.ts line 52 at r3 (raw file):
Previously, THardy98 (Thomas Hardy) wrote…
This isn't entirely accurate anymore, we only throw an error when the request succeeds with empty results and throws an error.
Done.
pkg/ui/workspaces/cluster-ui/src/databasesPage/databasesPage.tsx line 303 at r3 (raw file):
Previously, THardy98 (Thomas Hardy) wrote…
Do we want to log on refresh? We can probably remove this.
Good catch, I thought I removed all the console logs.
pkg/ui/workspaces/cluster-ui/src/databasesPage/databasesPage.tsx line 307 at r3 (raw file):
Previously, THardy98 (Thomas Hardy) wrote…
We are reassigning this immediately below, we can initialize instead as:
let startIndex = this.state.pagination.pageSize * (this.state.pagination.current - 1);
Done.
pkg/ui/workspaces/cluster-ui/src/databasesPage/databasesPage.tsx line 315 at r3 (raw file):
Previously, THardy98 (Thomas Hardy) wrote…
Do we need the
!filteredDbscheck if we assign it to all databases by default?
The issue is if a user searches for a name that does not exist it can return no results. Which can cause the sort and foreach to fail.
pkg/ui/workspaces/cluster-ui/src/databasesPage/databasesPage.tsx line 335 at r3 (raw file):
Previously, THardy98 (Thomas Hardy) wrote…
We don't seem to be using the
iorarrparams, can just be(database).
Done.
pkg/ui/workspaces/cluster-ui/src/sortedtable/sortedtable.tsx line 106 at r3 (raw file):
Previously, THardy98 (Thomas Hardy) wrote…
Maybe could use a better name,
disableSortForLargeDatasounds like a boolean value.disableSortSizeLimitor something similar might be more intuitive.
Done.
pkg/ui/workspaces/cluster-ui/src/sortedtable/sortedtable.tsx line 231 at r3 (raw file):
Previously, THardy98 (Thomas Hardy) wrote…
disableSortForLargeDataisn't an optional parameter, are we checking for a non-zero value?
We're comparingdata.lengthto it anyways in the conditional below so not sure if this check is necessary.
I made the parameter optional. Even though it's not optional currently it doesn't fail to build for any of the other tables and the value is undefined for other tables.
pkg/ui/workspaces/cluster-ui/src/sortedtable/sortedtable.tsx line 266 at r3 (raw file):
Previously, THardy98 (Thomas Hardy) wrote…
Ditto.
It can be undefined so it needs the check.
There was a problem hiding this comment.
Reviewable status:
|
Why do we need to check specifically if it's |
|
Previously, xinhaoz (Xin Hao Zhang) wrote…
GetDatabaseInfoError has the full error message already formatted for the user, and it shouldn't show a refresh option. Other errors should show the existing error message and refresh button. |
|
bors r+ |
|
Build succeeded: |
j82w
changed the title
The databases page displays partial results instead of
just showing an error message.
Sorting is disabled if there are more than 2 pages of results
which is currently configured to 40dbs. This still allows most
user to use sort functionality, but prevents large customers
from breaking when it would need to do a network call per a
database.
The database details are now loaded on demand for the first
page only. Previously a network call was done for all databases
which would result in 2k network calls. It now only loads the page
of details the user is looking at.
part of: #94333
https://www.loom.com/share/31b213b2f1764d0f9868bd967b9388b8
Release note: none