Skip to content
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.

Sign up for GitHub

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: fix inflight request replacement, cleanup database details api #99095

Merged
merged 1 commit into from
Mar 21, 2023
Merged

ui: fix inflight request replacement, cleanup database details api #99095

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
109 changes: 48 additions & 61 deletions pkg/ui/workspaces/cluster-ui/src/api/databaseDetailsApi.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,39 +33,38 @@ type ZoneConfigType = cockroach.config.zonepb.ZoneConfig;
type ZoneConfigLevelType = cockroach.server.serverpb.ZoneConfigurationLevel;

export type DatabaseDetailsResponse = {
id_resp: SqlApiQueryResponse<DatabaseIdRow>;
grants_resp: SqlApiQueryResponse<DatabaseGrantsResponse>;
tables_resp: SqlApiQueryResponse<DatabaseTablesResponse>;
zone_config_resp: SqlApiQueryResponse<DatabaseZoneConfigResponse>;
idResp: SqlApiQueryResponse<DatabaseIdRow>;
grantsResp: SqlApiQueryResponse<DatabaseGrantsResponse>;
tablesResp: SqlApiQueryResponse<DatabaseTablesResponse>;
zoneConfigResp: SqlApiQueryResponse<DatabaseZoneConfigResponse>;
stats?: DatabaseDetailsStats;
error?: SqlExecutionErrorMessage;
};

function newDatabaseDetailsResponse(): DatabaseDetailsResponse {
return {
id_resp: { database_id: "" },
grants_resp: { grants: [] },
tables_resp: { tables: [] },
zone_config_resp: {
idResp: { database_id: "" },
grantsResp: { grants: [] },
tablesResp: { tables: [] },
zoneConfigResp: {
zone_config: new ZoneConfig({
inherited_constraints: true,
inherited_lease_preferences: true,
}),
zone_config_level: ZoneConfigurationLevel.CLUSTER,
},
stats: {
ranges_data: {
range_count: 0,
spanStats: {
approximate_disk_bytes: 0,
live_bytes: 0,
total_bytes: 0,
// Note: we are currently populating this with replica ids which do not map 1 to 1
node_ids: [],
regions: [],
range_count: 0,
},
pebble_data: {
approximate_disk_bytes: 0,
replicaData: {
replicas: [],
regions: [],
},
index_stats: { num_index_recommendations: 0 },
indexStats: { num_index_recommendations: 0 },
},
};
}
Expand All @@ -88,10 +87,10 @@ const getDatabaseId: DatabaseDetailsQuery<DatabaseIdRow> = {
) => {
// Check that txn_result.rows[0].database_id is not null
if (!txnResultIsEmpty(txn_result) && txn_result.rows[0].database_id) {
resp.id_resp.database_id = txn_result.rows[0].database_id;
resp.idResp.database_id = txn_result.rows[0].database_id;
}
if (txn_result.error) {
resp.id_resp.error = txn_result.error;
resp.idResp.error = txn_result.error;
}
},
};
Expand All @@ -109,9 +108,12 @@ type DatabaseGrantsRow = {
const getDatabaseGrantsQuery: DatabaseDetailsQuery<DatabaseGrantsRow> = {
createStmt: dbName => {
return {
sql: `SELECT grantee as user, array_agg(privilege_type) as privileges
FROM crdb_internal.cluster_database_privileges
sql: Format(
`SELECT grantee as user, array_agg(privilege_type) as privileges
FROM %1.crdb_internal.cluster_database_privileges
WHERE database_name = $1 group by grantee`,
[new Identifier(dbName)],
),
arguments: [dbName],
};
},
Expand All @@ -120,9 +122,9 @@ const getDatabaseGrantsQuery: DatabaseDetailsQuery<DatabaseGrantsRow> = {
resp: DatabaseDetailsResponse,
) => {
if (!txnResultIsEmpty(txn_result)) {
resp.grants_resp.grants = txn_result.rows;
resp.grantsResp.grants = txn_result.rows;
if (txn_result.error) {
resp.grants_resp.error = txn_result.error;
resp.grantsResp.error = txn_result.error;
}
}
},
Expand Down Expand Up @@ -155,7 +157,7 @@ const getDatabaseTablesQuery: DatabaseDetailsQuery<DatabaseTablesRow> = {
resp: DatabaseDetailsResponse,
) => {
if (!txnResultIsEmpty(txn_result)) {
resp.tables_resp.tables = txn_result.rows.map(row => {
resp.tablesResp.tables = txn_result.rows.map(row => {
const escTableName = new QualifiedIdentifier([
row.table_schema,
row.table_name,
Expand All @@ -164,7 +166,7 @@ const getDatabaseTablesQuery: DatabaseDetailsQuery<DatabaseTablesRow> = {
});
}
if (txn_result.error) {
resp.tables_resp.error = txn_result.error;
resp.tablesResp.error = txn_result.error;
}
},
};
Expand Down Expand Up @@ -209,47 +211,32 @@ const getDatabaseZoneConfig: DatabaseDetailsQuery<DatabaseZoneConfigRow> = {
// Parse the bytes from the hex string.
const zoneConfigBytes = fromHexString(zoneConfigHexString);
// Decode the bytes using ZoneConfig protobuf.
resp.zone_config_resp.zone_config = ZoneConfig.decode(
resp.zoneConfigResp.zone_config = ZoneConfig.decode(
new Uint8Array(zoneConfigBytes),
);
resp.zone_config_resp.zone_config_level =
ZoneConfigurationLevel.DATABASE;
resp.zoneConfigResp.zone_config_level = ZoneConfigurationLevel.DATABASE;
} catch (e) {
console.error(
`Database Details API - encountered an error decoding zone config string: ${zoneConfigHexString}`,
);
// Catch and assign the error if we encounter one decoding.
resp.zone_config_resp.error = e;
resp.zone_config_resp.zone_config_level =
ZoneConfigurationLevel.UNKNOWN;
resp.zoneConfigResp.error = e;
resp.zoneConfigResp.zone_config_level = ZoneConfigurationLevel.UNKNOWN;
}
}
if (txn_result.error) {
resp.id_resp.error = txn_result.error;
resp.idResp.error = txn_result.error;
}
},
};

// Database Stats
type DatabaseDetailsStats = {
pebble_data: DatabasePebbleData;
ranges_data: DatabaseRangesData;
index_stats: DatabaseIndexUsageStatsResponse;
spanStats: SqlApiQueryResponse<DatabaseSpanStatsRow>;
replicaData: SqlApiQueryResponse<DatabaseReplicasRegionsRow>;
indexStats: SqlApiQueryResponse<DatabaseIndexUsageStatsResponse>;
};

type DatabasePebbleData = {
approximate_disk_bytes: number;
};

type DatabaseRangesData = SqlApiQueryResponse<{
range_count: number;
live_bytes: number;
total_bytes: number;
// Note: we are currently populating this with replica ids which do not map 1 to 1
node_ids: number[];
regions: string[];
}>;

type DatabaseSpanStatsRow = {
approximate_disk_bytes: number;
live_bytes: number;
Expand All @@ -274,20 +261,19 @@ const getDatabaseSpanStats: DatabaseDetailsQuery<DatabaseSpanStatsRow> = {
resp: DatabaseDetailsResponse,
) => {
if (txn_result && txn_result.error) {
resp.stats.ranges_data.error = txn_result.error;
resp.stats.spanStats.error = txn_result.error;
}
if (txnResultIsEmpty(txn_result)) {
return;
}
if (txn_result.rows.length === 1) {
const row = txn_result.rows[0];
resp.stats.pebble_data.approximate_disk_bytes =
row.approximate_disk_bytes;
resp.stats.ranges_data.range_count = row.range_count;
resp.stats.ranges_data.live_bytes = row.live_bytes;
resp.stats.ranges_data.total_bytes = row.total_bytes;
resp.stats.spanStats.approximate_disk_bytes = row.approximate_disk_bytes;
resp.stats.spanStats.range_count = row.range_count;
resp.stats.spanStats.live_bytes = row.live_bytes;
resp.stats.spanStats.total_bytes = row.total_bytes;
} else {
resp.stats.ranges_data.error = new Error(
resp.stats.spanStats.error = new Error(
`DatabaseDetails - Span Stats, expected 1 row, got ${txn_result.rows.length}`,
);
}
Expand Down Expand Up @@ -327,18 +313,18 @@ const getDatabaseReplicasAndRegions: DatabaseDetailsQuery<DatabaseReplicasRegion
resp: DatabaseDetailsResponse,
) => {
if (!txnResultIsEmpty(txn_result)) {
resp.stats.ranges_data.regions = txn_result.rows[0].regions;
resp.stats.ranges_data.node_ids = txn_result.rows[0].replicas;
resp.stats.replicaData.regions = txn_result.rows[0].regions;
resp.stats.replicaData.replicas = txn_result.rows[0].replicas;
}
if (txn_result.error) {
resp.stats.ranges_data.error = txn_result.error;
resp.stats.replicaData.error = txn_result.error;
}
},
};

type DatabaseIndexUsageStatsResponse = SqlApiQueryResponse<{
type DatabaseIndexUsageStatsResponse = {
num_index_recommendations?: number;
}>;
};

const getDatabaseIndexUsageStats: DatabaseDetailsQuery<IndexUsageStatistic> = {
createStmt: dbName => {
Expand Down Expand Up @@ -374,11 +360,11 @@ const getDatabaseIndexUsageStats: DatabaseDetailsQuery<IndexUsageStatistic> = {
txn_result.rows?.forEach(row => {
const rec = recommendDropUnusedIndex(row);
if (rec.recommend) {
resp.stats.index_stats.num_index_recommendations += 1;
resp.stats.indexStats.num_index_recommendations += 1;
}
});
if (txn_result.error) {
resp.stats.index_stats.error = txn_result.error;
resp.stats.indexStats.error = txn_result.error;
}
},
};
Expand Down Expand Up @@ -414,6 +400,7 @@ export function createDatabaseDetailsReq(dbName: string): SqlExecutionRequest {
return {
execute: true,
statements: databaseDetailQueries.map(query => query.createStmt(dbName)),
database: dbName,
max_result_size: LARGE_RESULT_SIZE,
timeout: LONG_TIMEOUT,
};
Expand Down
2 changes: 1 addition & 1 deletion pkg/ui/workspaces/cluster-ui/src/sql/highlight.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ export class Highlight extends React.Component<SqlBoxProps> {

renderZone = (): React.ReactElement => {
const { zone } = this.props;
const zoneConfig = zone.zone_config_resp.zone_config;
const zoneConfig = zone.zoneConfigResp.zone_config;
return (
<span className={cx("sql-highlight", "hljs")}>
<span className="hljs-keyword">CONFIGURE ZONE USING</span>
Expand Down
19 changes: 15 additions & 4 deletions pkg/ui/workspaces/db-console/src/redux/cachedDataReducer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,14 +297,22 @@ export class CachedDataReducer<
return this.apiEndpoint(req, this.requestTimeout)
.then(
data => {
// Dispatch the results to the store.
if (this.pendingRequestStarted !== pendingRequestStarted) {
// Check if we are replacing requests. If so, do not update the reducer
// state if this is not the latest request.
if (
this.allowReplacementRequests &&
this.pendingRequestStarted !== pendingRequestStarted
) {
return;
}
// Dispatch the results to the store.
dispatch(this.receiveData(data, req));
},
(error: Error) => {
if (this.pendingRequestStarted !== pendingRequestStarted) {
if (
this.allowReplacementRequests &&
this.pendingRequestStarted !== pendingRequestStarted
) {
return;
}

Expand All @@ -328,7 +336,10 @@ export class CachedDataReducer<
},
)
.then(() => {
if (this.pendingRequestStarted !== pendingRequestStarted) {
if (
this.allowReplacementRequests &&
this.pendingRequestStarted !== pendingRequestStarted
) {
return;
}
// Invalidate data after the invalidation period if one exists.
Expand Down
23 changes: 11 additions & 12 deletions pkg/ui/workspaces/db-console/src/util/api.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,7 @@ describe("rest api", function () {
},
],
},
// Database span stats query
{
rows: [
{
Expand All @@ -195,24 +196,22 @@ describe("rest api", function () {

return clusterUiApi.getDatabaseDetails(dbName).then(result => {
expect(fetchMock.calls(clusterUiApi.SQL_API_PATH).length).toBe(1);
expect(result.results.id_resp.database_id).toEqual("1");
expect(result.results.tables_resp.tables.length).toBe(1);
expect(result.results.grants_resp.grants.length).toBe(2);
expect(result.results.stats.index_stats.num_index_recommendations).toBe(
expect(result.results.idResp.database_id).toEqual("1");
expect(result.results.tablesResp.tables.length).toBe(1);
expect(result.results.grantsResp.grants.length).toBe(2);
expect(result.results.stats.indexStats.num_index_recommendations).toBe(
1,
);
expect(result.results.zone_config_resp.zone_config).toEqual(
expect(result.results.zoneConfigResp.zone_config).toEqual(
mockZoneConfig,
);
expect(result.results.zone_config_resp.zone_config_level).toBe(
expect(result.results.zoneConfigResp.zone_config_level).toBe(
ZoneConfigurationLevel.DATABASE,
);
expect(result.results.stats.pebble_data.approximate_disk_bytes).toBe(
100,
);
expect(result.results.stats.ranges_data.live_bytes).toBe(200);
expect(result.results.stats.ranges_data.total_bytes).toBe(300);
expect(result.results.stats.ranges_data.range_count).toBe(400);
expect(result.results.stats.spanStats.approximate_disk_bytes).toBe(100);
expect(result.results.stats.spanStats.live_bytes).toBe(200);
expect(result.results.stats.spanStats.total_bytes).toBe(300);
expect(result.results.stats.spanStats.range_count).toBe(400);
});
});

Expand Down
2 changes: 1 addition & 1 deletion pkg/ui/workspaces/db-console/src/views/databases/databaseDetailsPage/redux.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ export const mapStateToProps = createSelector(
nodeRegions: nodeRegions,
isTenant: isTenant,
tables: _.map(
databaseDetails[database]?.data?.results.tables_resp.tables,
databaseDetails[database]?.data?.results.tablesResp.tables,
table => {
const tableId = generateTableID(database, table);
const details = tableDetails[tableId];
Expand Down
11 changes: 6 additions & 5 deletions pkg/ui/workspaces/db-console/src/views/databases/databasesPage/redux.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,16 +81,17 @@ const selectDatabases = createSelector(
(databases?.databases || []).map(database => {
const details = databaseDetails[database];
const stats = details?.data?.results.stats;
const sizeInBytes = stats?.pebble_data?.approximate_disk_bytes || 0;
const rangeCount = stats?.ranges_data.range_count || 0;
const nodes = stats?.ranges_data.node_ids || [];
const sizeInBytes = stats?.spanStats?.approximate_disk_bytes || 0;
const rangeCount = stats?.spanStats.range_count || 0;
// TODO(thomas): Eventually, we should populate this will real node IDs.
const nodes = stats?.replicaData.replicas || [];
const nodesByRegionString = getNodesByRegionString(
nodes,
nodeRegions,
isTenant,
);
const numIndexRecommendations =
stats?.index_stats.num_index_recommendations || 0;
stats?.indexStats.num_index_recommendations || 0;

const combinedErr = combineLoadingErrors(
details?.lastError,
Expand All @@ -104,7 +105,7 @@ const selectDatabases = createSelector(
lastError: combinedErr,
name: database,
sizeInBytes: sizeInBytes,
tableCount: details?.data?.results.tables_resp.tables?.length || 0,
tableCount: details?.data?.results.tablesResp.tables?.length || 0,
rangeCount: rangeCount,
nodes: nodes,
nodesByRegionString,
Expand Down