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.

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

release-23.2: cluster-ui, db-console: fix bug where store ids are treated as node ids #120212

Merged
merged 1 commit into from Mar 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -43,6 +43,7 @@ import {
selectDropUnusedIndexDuration,
selectIndexRecommendationsEnabled,
} from "../store/clusterSettings/clusterSettings.selectors";
import { actions as nodesActions } from "../store/nodes/nodes.reducer";

const mapStateToProps = (
state: AppState,
Expand All @@ -54,6 +55,7 @@ const mapStateToProps = (
databaseDetails[database]?.data?.results.tablesResp.tables || [];
const nodeRegions = nodeRegionsByIDSelector(state);
const isTenant = selectIsTenant(state);
const nodeStatuses = state.adminUI?.nodes.data;
return {
loading: !!databaseDetails[database]?.inFlight,
loaded: !!databaseDetails[database]?.valid,
Expand All @@ -74,6 +76,7 @@ const mapStateToProps = (
tableDetails: state.adminUI?.tableDetails,
nodeRegions,
isTenant,
nodeStatuses,
}),
showIndexRecommendations: selectIndexRecommendationsEnabled(state),
csIndexUnusedDuration: selectDropUnusedIndexDuration(state),
Expand Down Expand Up @@ -174,6 +177,9 @@ const mapDispatchToProps = (
}),
);
},
refreshNodes: () => {
dispatch(nodesActions.refresh());
},
});

export const ConnectedDatabaseDetailsPage = withRouter(
Expand Down
Expand Up @@ -53,6 +53,7 @@ const withLoadingIndicator: DatabaseDetailsPageProps = {
onSortingGrantsChange: () => {},
refreshDatabaseDetails: () => {},
refreshTableDetails: () => {},
refreshNodes: () => {},
search: null,
filters: defaultFilters,
nodeRegions: {},
Expand Down Expand Up @@ -88,6 +89,7 @@ const withoutData: DatabaseDetailsPageProps = {
onSortingGrantsChange: () => {},
refreshDatabaseDetails: () => {},
refreshTableDetails: () => {},
refreshNodes: () => {},
search: null,
filters: defaultFilters,
nodeRegions: {},
Expand Down Expand Up @@ -163,6 +165,7 @@ const withData: DatabaseDetailsPageProps = {
onSortingGrantsChange: () => {},
refreshDatabaseDetails: () => {},
refreshTableDetails: () => {},
refreshNodes: () => {},
search: null,
filters: defaultFilters,
nodeRegions: {},
Expand Down
Expand Up @@ -170,6 +170,7 @@ export interface DatabaseDetailsPageActions {
onSortingTablesChange?: (columnTitle: string, ascending: boolean) => void;
onSortingGrantsChange?: (columnTitle: string, ascending: boolean) => void;
onViewModeChange?: (viewMode: ViewMode) => void;
refreshNodes: () => void;
}

export type DatabaseDetailsPageProps = DatabaseDetailsPageData &
Expand Down Expand Up @@ -264,6 +265,7 @@ export class DatabaseDetailsPage extends React.Component<
}

componentDidMount(): void {
this.props.refreshNodes();
if (!this.props.loaded && !this.props.loading && !this.props.requestError) {
this.props.refreshDatabaseDetails(
this.props.name,
Expand Down
Expand Up @@ -65,11 +65,17 @@ export const mapStateToProps = (
);
const nodeRegions = nodeRegionsByIDSelector(state);
const isTenant = selectIsTenant(state);
const nodeStatuses = state.adminUI?.nodes.data;
return {
databaseName: database,
name: table,
schemaName: schema,
details: deriveTablePageDetailsMemoized({ details, nodeRegions, isTenant }),
details: deriveTablePageDetailsMemoized({
details,
nodeRegions,
isTenant,
nodeStatuses,
}),
showNodeRegionsSection: Object.keys(nodeRegions).length > 1 && !isTenant,
automaticStatsCollectionEnabled:
selectAutomaticStatsCollectionEnabled(state),
Expand Down
74 changes: 56 additions & 18 deletions pkg/ui/workspaces/cluster-ui/src/databases/combiners.ts
Expand Up @@ -8,10 +8,13 @@
// by the Apache License, Version 2.0, included in the file
// licenses/APL.txt.

import { DatabasesListResponse, SqlExecutionErrorMessage } from "../api";
import { DatabasesListResponse } from "../api";
import { DatabasesPageDataDatabase } from "../databasesPage";
import {
Nodes,
Stores,
buildIndexStatToRecommendationsMap,
getNodeIdsFromStoreIds,
getNodesByRegionString,
normalizePrivileges,
normalizeRoles,
Expand All @@ -38,6 +41,8 @@ interface DerivedDatabaseDetailsParams {
spanStats: Record<string, DatabaseDetailsSpanStatsState>;
nodeRegions: Record<string, string>;
isTenant: boolean;
/** A list of node statuses so that store ids can be mapped to nodes. */
nodeStatuses: cockroach.server.status.statuspb.INodeStatus[];
}

export const deriveDatabaseDetailsMemoized = createSelector(
Expand All @@ -46,12 +51,14 @@ export const deriveDatabaseDetailsMemoized = createSelector(
(params: DerivedDatabaseDetailsParams) => params.spanStats,
(params: DerivedDatabaseDetailsParams) => params.nodeRegions,
(params: DerivedDatabaseDetailsParams) => params.isTenant,
(params: DerivedDatabaseDetailsParams) => params.nodeStatuses,
(
dbListResp,
databaseDetails,
spanStats,
nodeRegions,
isTenant,
nodeStatuses,
): DatabasesPageDataDatabase[] => {
const databases = dbListResp?.databases ?? [];
return databases.map(dbName => {
Expand All @@ -61,9 +68,9 @@ export const deriveDatabaseDetailsMemoized = createSelector(
dbName,
dbDetails,
spanStatsForDB,
dbListResp.error,
nodeRegions,
isTenant,
nodeStatuses,
);
});
},
Expand All @@ -73,15 +80,22 @@ const deriveDatabaseDetails = (
database: string,
dbDetails: DatabaseDetailsState,
spanStats: DatabaseDetailsSpanStatsState,
dbListError: SqlExecutionErrorMessage,
nodeRegionsByID: Record<string, string>,
isTenant: boolean,
nodeStatuses: cockroach.server.status.statuspb.INodeStatus[],
): DatabasesPageDataDatabase => {
const dbStats = dbDetails?.data?.results.stats;
// TODO #118957 (xinhaoz) Use store id to regions mapping.
const stores = dbStats?.replicaData.storeIDs || [];
/** List of store IDs for the current cluster. All of the values in the
* `*replicas` columns correspond to store IDs. */
const stores: Stores = {
kind: "store",
ids: dbStats?.replicaData.storeIDs || [],
};
/** List of node IDs for the current cluster. */
const nodes = getNodeIdsFromStoreIds(stores, nodeStatuses);

const nodesByRegionString = getNodesByRegionString(
stores,
nodes,
nodeRegionsByID,
isTenant,
);
Expand All @@ -100,7 +114,7 @@ const deriveDatabaseDetails = (
name: database,
spanStats: spanStats?.data?.results.spanStats,
tables: dbDetails?.data?.results.tablesResp,
nodes: stores,
nodes: nodes.ids,
nodesByRegionString,
numIndexRecommendations,
};
Expand All @@ -112,6 +126,8 @@ interface DerivedTableDetailsParams {
tableDetails: Record<string, TableDetailsState>;
nodeRegions: Record<string, string>;
isTenant: boolean;
/** A list of node statuses so that store ids can be mapped to nodes. */
nodeStatuses: cockroach.server.status.statuspb.INodeStatus[];
}

export const deriveTableDetailsMemoized = createSelector(
Expand All @@ -120,18 +136,26 @@ export const deriveTableDetailsMemoized = createSelector(
(params: DerivedTableDetailsParams) => params.tableDetails,
(params: DerivedTableDetailsParams) => params.nodeRegions,
(params: DerivedTableDetailsParams) => params.isTenant,
(params: DerivedTableDetailsParams) => params.nodeStatuses,
(
dbName,
tables,
tableDetails,
nodeRegions,
isTenant,
nodeStatuses,
): DatabaseDetailsPageDataTable[] => {
tables = tables || [];
return tables.map(table => {
const tableID = generateTableID(dbName, table);
const details = tableDetails[tableID];
return deriveDatabaseTableDetails(table, details, nodeRegions, isTenant);
return deriveDatabaseTableDetails(
table,
details,
nodeRegions,
isTenant,
nodeStatuses,
);
});
},
);
Expand All @@ -141,14 +165,19 @@ const deriveDatabaseTableDetails = (
details: TableDetailsState,
nodeRegions: Record<string, string>,
isTenant: boolean,
nodeStatuses: cockroach.server.status.statuspb.INodeStatus[],
): DatabaseDetailsPageDataTable => {
const results = details?.data?.results;
const grants = results?.grantsResp.grants ?? [];
const normalizedRoles = normalizeRoles(grants.map(grant => grant.user));
const normalizedPrivileges = normalizePrivileges(
[].concat(...grants.map(grant => grant.privileges)),
);
const storeIDs = results?.stats.replicaData.storeIDs || [];
const stores: Stores = {
kind: "store",
ids: results?.stats.replicaData.storeIDs || [],
};
const nodes: Nodes = getNodeIdsFromStoreIds(stores, nodeStatuses);
return {
name: table,
loading: !!details?.inFlight,
Expand All @@ -165,13 +194,8 @@ const deriveDatabaseTableDetails = (
statsLastUpdated: results?.heuristicsDetails,
indexStatRecs: results?.stats.indexStats,
spanStats: results?.stats.spanStats,
// TODO #118957 (xinhaoz) Store IDs and node IDs cannot be used interchangeably.
nodes: storeIDs,
nodesByRegionString: getNodesByRegionString(
storeIDs,
nodeRegions,
isTenant,
),
nodes: nodes.ids,
nodesByRegionString: getNodesByRegionString(nodes, nodeRegions, isTenant),
},
};
};
Expand All @@ -180,21 +204,35 @@ interface DerivedTablePageDetailsParams {
details: TableDetailsState;
nodeRegions: Record<string, string>;
isTenant: boolean;
/** A list of node statuses so that store ids can be mapped to nodes. */
nodeStatuses: cockroach.server.status.statuspb.INodeStatus[];
}

export const deriveTablePageDetailsMemoized = createSelector(
(params: DerivedTablePageDetailsParams) => params.details,
(params: DerivedTablePageDetailsParams) => params.nodeRegions,
(params: DerivedTablePageDetailsParams) => params.isTenant,
(details, nodeRegions, isTenant): DatabaseTablePageDataDetails => {
(params: DerivedTablePageDetailsParams) => params.nodeStatuses,
(
details,
nodeRegions,
isTenant,
nodeStatuses,
): DatabaseTablePageDataDetails => {
const results = details?.data?.results;
const grants = results?.grantsResp.grants || [];
const normalizedGrants =
grants.map(grant => ({
user: grant.user,
privileges: normalizePrivileges(grant.privileges),
})) || [];
const nodes = results?.stats.replicaData.storeIDs || [];

const stores: Stores = {
kind: "store",
ids: results?.stats.replicaData.storeIDs || [],
};
const nodes = getNodeIdsFromStoreIds(stores, nodeStatuses);

return {
loading: !!details?.inFlight,
loaded: !!details?.valid,
Expand Down