From bd18e4c7e133353b8dc570616c3632907dfa8a13 Mon Sep 17 00:00:00 2001 From: Michael Erickson Date: Mon, 28 Oct 2024 12:13:48 -0700 Subject: [PATCH 1/4] logic_test: add retry to SHOW CLUSTER SETTING The cluster setting system is not synchronous; sometimes there is a delay between writing to system.settings and seeing the change in SHOW CLUSTER SETTING output. Fixes: #133429 Release note: None --- pkg/sql/logictest/testdata/logic_test/system | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/sql/logictest/testdata/logic_test/system b/pkg/sql/logictest/testdata/logic_test/system index 9018c907c3dd..7acb9eb6fc7c 100644 --- a/pkg/sql/logictest/testdata/logic_test/system +++ b/pkg/sql/logictest/testdata/logic_test/system @@ -1440,7 +1440,7 @@ SELECT name, value FROM system.settings WHERE name = 'sql.defaults.vectorize' ---- sql.defaults.vectorize 1 -query T +query T retry SHOW CLUSTER SETTING sql.defaults.vectorize ---- on @@ -1468,7 +1468,7 @@ query TT SELECT name, value FROM system.settings WHERE name = 'sql.defaults.vectorize' ---- -query T +query T retry SHOW CLUSTER SETTING sql.defaults.vectorize ---- on From c09ef5db2530918f9d088a9f62384be37915e7ab Mon Sep 17 00:00:00 2001 From: Xin Hao Zhang Date: Fri, 25 Oct 2024 15:00:06 -0400 Subject: [PATCH 2/4] ui: cleanup useNodeStatuses usages This commit cleans up the useNodeStatuses hook. The raw data from the response is no longer returned from the hook. Instead, callers must use the transformed data. Epic: none Release note: None --- .../workspaces/cluster-ui/src/api/nodesApi.ts | 29 +++++---- .../nodeRegionsSelector.spec.tsx | 63 +++++++++++-------- .../nodeRegionsSelector.tsx | 17 ++--- .../src/databaseDetailsV2/tablesView.tsx | 4 +- .../src/databaseDetailsV2/utils.tsx | 5 +- .../cluster-ui/src/databasesV2/index.tsx | 2 +- .../cluster-ui/src/databasesV2/utils.ts | 5 +- .../src/tableDetailsV2/tableOverview.tsx | 4 +- .../cluster-ui/src/util/nodeUtils.spec.ts | 15 ++--- .../cluster-ui/src/util/nodeUtils.ts | 5 +- 10 files changed, 86 insertions(+), 63 deletions(-) diff --git a/pkg/ui/workspaces/cluster-ui/src/api/nodesApi.ts b/pkg/ui/workspaces/cluster-ui/src/api/nodesApi.ts index efafac4d729b..e1831a077a58 100644 --- a/pkg/ui/workspaces/cluster-ui/src/api/nodesApi.ts +++ b/pkg/ui/workspaces/cluster-ui/src/api/nodesApi.ts @@ -20,6 +20,11 @@ export const getNodes = return fetchData(cockroach.server.serverpb.NodesResponse, NODES_PATH); }; +export type NodeStatus = { + region: string; + stores: StoreID[]; +}; + export const useNodeStatuses = () => { const clusterDetails = useContext(ClusterDetailsContext); const isTenant = clusterDetails.isTenant; @@ -32,29 +37,31 @@ export const useNodeStatuses = () => { }, ); - const { storeIDToNodeID, nodeIDToRegion } = useMemo(() => { - const nodeIDToRegion: Record = {}; + const { storeIDToNodeID, nodeStatusByID } = useMemo(() => { + const nodeStatusByID: Record = {}; const storeIDToNodeID: Record = {}; if (!data) { - return { nodeIDToRegion, storeIDToNodeID }; + return { nodeStatusByID, storeIDToNodeID }; } - data.nodes.forEach(ns => { - ns.store_statuses.forEach(store => { + data.nodes?.forEach(ns => { + ns.store_statuses?.forEach(store => { storeIDToNodeID[store.desc.store_id as StoreID] = ns.desc .node_id as NodeID; }); - nodeIDToRegion[ns.desc.node_id as NodeID] = getRegionFromLocality( - ns.desc.locality, - ); + + nodeStatusByID[ns.desc.node_id as NodeID] = { + region: getRegionFromLocality(ns.desc.locality), + stores: ns.store_statuses?.map(s => s.desc.store_id as StoreID), + }; }); - return { nodeIDToRegion, storeIDToNodeID }; + + return { nodeStatusByID, storeIDToNodeID }; }, [data]); return { - data, isLoading, error, - nodeIDToRegion, + nodeStatusByID, storeIDToNodeID, }; }; diff --git a/pkg/ui/workspaces/cluster-ui/src/components/nodeRegionsSelector/nodeRegionsSelector.spec.tsx b/pkg/ui/workspaces/cluster-ui/src/components/nodeRegionsSelector/nodeRegionsSelector.spec.tsx index 51046fe0d06d..33242cce3434 100644 --- a/pkg/ui/workspaces/cluster-ui/src/components/nodeRegionsSelector/nodeRegionsSelector.spec.tsx +++ b/pkg/ui/workspaces/cluster-ui/src/components/nodeRegionsSelector/nodeRegionsSelector.spec.tsx @@ -3,54 +3,54 @@ // Use of this software is governed by the CockroachDB Software License // included in the /LICENSE file. +import { cockroach } from "@cockroachlabs/crdb-protobuf-client"; import { render, screen, fireEvent, waitFor } from "@testing-library/react"; import React from "react"; -import { useNodeStatuses } from "src/api"; -import { getRegionFromLocality } from "src/store/nodes"; -import { StoreID } from "src/types/clusterTypes"; +import * as api from "src/api/nodesApi"; +import { NodeID, StoreID } from "src/types/clusterTypes"; import { NodeRegionsSelector } from "./nodeRegionsSelector"; -// Mock the useNodeStatuses hook -jest.mock("src/api", () => ({ - useNodeStatuses: jest.fn(), -})); +import NodesResponse = cockroach.server.serverpb.NodesResponse; -// Mock the getRegionFromLocality function -jest.mock("src/store/nodes", () => ({ - getRegionFromLocality: jest.fn(), -})); - -const mockNodeData = { +const mockNodeData = new NodesResponse({ nodes: [ { - desc: { node_id: 1, locality: { region: "us-east" } }, + desc: { + node_id: 1, + locality: { tiers: [{ key: "region", value: "us-east" }] }, + }, store_statuses: [ { desc: { store_id: 101 } }, { desc: { store_id: 102 } }, ], }, { - desc: { node_id: 2, locality: { region: "us-west" } }, + desc: { + node_id: 2, + locality: { tiers: [{ key: "region", value: "us-west" }] }, + }, store_statuses: [{ desc: { store_id: 201 } }], }, { - desc: { node_id: 3, locality: { region: "us-east" } }, + desc: { + node_id: 3, + locality: { tiers: [{ key: "region", value: "us-east" }] }, + }, store_statuses: [{ desc: { store_id: 301 } }], }, ], -}; +}); describe("NodeRegionsSelector", () => { beforeEach(() => { - (useNodeStatuses as jest.Mock).mockReturnValue({ - isLoading: false, - data: mockNodeData, - }); - (getRegionFromLocality as jest.Mock).mockImplementation( - locality => locality.region, - ); + // Mock the api.getNodes function at the module level + jest.spyOn(api, "getNodes").mockResolvedValue(mockNodeData); + }); + + afterEach(() => { + jest.clearAllMocks(); }); it("should render", () => { @@ -103,9 +103,20 @@ describe("NodeRegionsSelector", () => { }); it("handles loading state", () => { - (useNodeStatuses as jest.Mock).mockReturnValue({ + jest.spyOn(api, "useNodeStatuses").mockReturnValue({ + error: null, isLoading: true, - data: mockNodeData, + nodeStatusByID: { + 1: { region: "us-east", stores: [101, 102] }, + 2: { region: "us-west", stores: [201] }, + 3: { region: "us-east", stores: [301] }, + } as Record, + storeIDToNodeID: { + 101: 1, + 102: 1, + 201: 2, + 301: 3, + } as Record, }); render( {}} />); diff --git a/pkg/ui/workspaces/cluster-ui/src/components/nodeRegionsSelector/nodeRegionsSelector.tsx b/pkg/ui/workspaces/cluster-ui/src/components/nodeRegionsSelector/nodeRegionsSelector.tsx index da8ab64d0a9d..93bad55f6cb1 100644 --- a/pkg/ui/workspaces/cluster-ui/src/components/nodeRegionsSelector/nodeRegionsSelector.tsx +++ b/pkg/ui/workspaces/cluster-ui/src/components/nodeRegionsSelector/nodeRegionsSelector.tsx @@ -7,7 +7,6 @@ import React, { useMemo } from "react"; import Select, { OptionsType } from "react-select"; import { useNodeStatuses } from "src/api"; -import { getRegionFromLocality } from "src/store/nodes"; import { NodeID, StoreID } from "src/types/clusterTypes"; import { GroupedReactSelectOption, @@ -23,22 +22,24 @@ export const NodeRegionsSelector: React.FC = ({ value, onChange, }) => { - const nodesResp = useNodeStatuses(); + const { nodeStatusByID, isLoading } = useNodeStatuses(); const nodeOptions: GroupedReactSelectOption[] = useMemo(() => { const optionsMap: Record = {}; - if (nodesResp.isLoading && !nodesResp.data?.nodes) { + const nodes = Object.keys(nodeStatusByID ?? {}); + if (isLoading && !nodes.length) { return []; } - nodesResp.data.nodes.forEach(node => { - const region = getRegionFromLocality(node.desc.locality); + nodes.forEach(node => { + const nid = parseInt(node) as NodeID; + const region = nodeStatusByID[nid].region; if (optionsMap[region] == null) { optionsMap[region] = []; } optionsMap[region].push({ - nid: node.desc.node_id as NodeID, - sids: node.store_statuses.map(s => s.desc.store_id as StoreID), + nid, + sids: nodeStatusByID[nid].stores, }); }); @@ -51,7 +52,7 @@ export const NodeRegionsSelector: React.FC = ({ })), }; }); - }, [nodesResp]); + }, [nodeStatusByID, isLoading]); const onSelectChange = ( selected: OptionsType>, diff --git a/pkg/ui/workspaces/cluster-ui/src/databaseDetailsV2/tablesView.tsx b/pkg/ui/workspaces/cluster-ui/src/databaseDetailsV2/tablesView.tsx index f727090ddf10..80acf85cc348 100644 --- a/pkg/ui/workspaces/cluster-ui/src/databaseDetailsV2/tablesView.tsx +++ b/pkg/ui/workspaces/cluster-ui/src/databaseDetailsV2/tablesView.tsx @@ -240,13 +240,13 @@ export const TablesPageV2 = () => { const tableData = useMemo( () => tableMetadataToRows(tableList ?? [], { - nodeIDToRegion: nodesResp.nodeIDToRegion, + nodeStatusByID: nodesResp.nodeStatusByID, storeIDToNodeID: nodesResp.storeIDToNodeID, isLoading: nodesResp.isLoading, }), [ tableList, - nodesResp.nodeIDToRegion, + nodesResp.nodeStatusByID, nodesResp.storeIDToNodeID, nodesResp.isLoading, ], diff --git a/pkg/ui/workspaces/cluster-ui/src/databaseDetailsV2/utils.tsx b/pkg/ui/workspaces/cluster-ui/src/databaseDetailsV2/utils.tsx index 6aa92998738a..6704ad0bb395 100644 --- a/pkg/ui/workspaces/cluster-ui/src/databaseDetailsV2/utils.tsx +++ b/pkg/ui/workspaces/cluster-ui/src/databaseDetailsV2/utils.tsx @@ -3,6 +3,7 @@ // Use of this software is governed by the CockroachDB Software License // included in the /LICENSE file. +import { NodeStatus } from "src/api"; import { TableMetadata } from "src/api/databases/getTableMetadataApi"; import { NodeID, StoreID } from "src/types/clusterTypes"; import { mapStoreIDsToNodeRegions } from "src/util/nodeUtils"; @@ -12,7 +13,7 @@ import { TableRow } from "./types"; export const tableMetadataToRows = ( tables: TableMetadata[], nodesInfo: { - nodeIDToRegion: Record; + nodeStatusByID: Record; storeIDToNodeID: Record; isLoading: boolean; }, @@ -20,7 +21,7 @@ export const tableMetadataToRows = ( return tables.map(table => { const nodesByRegion = mapStoreIDsToNodeRegions( table.storeIds, - nodesInfo?.nodeIDToRegion, + nodesInfo?.nodeStatusByID, nodesInfo?.storeIDToNodeID, ); return { diff --git a/pkg/ui/workspaces/cluster-ui/src/databasesV2/index.tsx b/pkg/ui/workspaces/cluster-ui/src/databasesV2/index.tsx index 417f2d3270e2..fffca65a35d1 100644 --- a/pkg/ui/workspaces/cluster-ui/src/databasesV2/index.tsx +++ b/pkg/ui/workspaces/cluster-ui/src/databasesV2/index.tsx @@ -159,7 +159,7 @@ export const DatabasesPageV2 = () => { const tableData = useMemo( () => rawDatabaseMetadataToDatabaseRows(data?.results ?? [], { - nodeIDToRegion: nodesResp.nodeIDToRegion, + nodeStatusByID: nodesResp.nodeStatusByID, storeIDToNodeID: nodesResp.storeIDToNodeID, isLoading: nodesResp.isLoading, }), diff --git a/pkg/ui/workspaces/cluster-ui/src/databasesV2/utils.ts b/pkg/ui/workspaces/cluster-ui/src/databasesV2/utils.ts index ec3caa6f6474..eb8bfcca3828 100644 --- a/pkg/ui/workspaces/cluster-ui/src/databasesV2/utils.ts +++ b/pkg/ui/workspaces/cluster-ui/src/databasesV2/utils.ts @@ -4,6 +4,7 @@ // included in the /LICENSE file. import { DatabaseMetadata } from "src/api/databases/getDatabaseMetadataApi"; +import { NodeStatus } from "src/api/nodesApi"; import { NodeID, StoreID } from "src/types/clusterTypes"; import { mapStoreIDsToNodeRegions } from "src/util/nodeUtils"; @@ -12,7 +13,7 @@ import { DatabaseRow } from "./databaseTypes"; export const rawDatabaseMetadataToDatabaseRows = ( raw: DatabaseMetadata[], nodesInfo: { - nodeIDToRegion: Record; + nodeStatusByID: Record; storeIDToNodeID: Record; isLoading: boolean; }, @@ -20,7 +21,7 @@ export const rawDatabaseMetadataToDatabaseRows = ( return raw.map((db: DatabaseMetadata): DatabaseRow => { const nodesByRegion = mapStoreIDsToNodeRegions( db.storeIds, - nodesInfo?.nodeIDToRegion, + nodesInfo?.nodeStatusByID, nodesInfo?.storeIDToNodeID, ); return { diff --git a/pkg/ui/workspaces/cluster-ui/src/tableDetailsV2/tableOverview.tsx b/pkg/ui/workspaces/cluster-ui/src/tableDetailsV2/tableOverview.tsx index ee719485f83f..7063370b019e 100644 --- a/pkg/ui/workspaces/cluster-ui/src/tableDetailsV2/tableOverview.tsx +++ b/pkg/ui/workspaces/cluster-ui/src/tableDetailsV2/tableOverview.tsx @@ -29,7 +29,7 @@ export const TableOverview: React.FC = ({ const isTenant = clusterDetails.isTenant; const metadata = tableDetails.metadata; const { - nodeIDToRegion, + nodeStatusByID, storeIDToNodeID, isLoading: nodesLoading, } = useNodeStatuses(); @@ -42,7 +42,7 @@ export const TableOverview: React.FC = ({ } const regionsToNodes = mapStoreIDsToNodeRegions( tableDetails.metadata.storeIds, - nodeIDToRegion, + nodeStatusByID, storeIDToNodeID, ); return Object.entries(regionsToNodes) diff --git a/pkg/ui/workspaces/cluster-ui/src/util/nodeUtils.spec.ts b/pkg/ui/workspaces/cluster-ui/src/util/nodeUtils.spec.ts index e46166285edf..c54f25f8a9aa 100644 --- a/pkg/ui/workspaces/cluster-ui/src/util/nodeUtils.spec.ts +++ b/pkg/ui/workspaces/cluster-ui/src/util/nodeUtils.spec.ts @@ -3,7 +3,8 @@ // Use of this software is governed by the CockroachDB Software License // included in the /LICENSE file. -import { StoreID } from "src/types/clusterTypes"; +import { NodeStatus } from "src/api"; +import { NodeID, StoreID } from "src/types/clusterTypes"; import { mapStoreIDsToNodeRegions } from "./nodeUtils"; @@ -12,12 +13,12 @@ describe("nodeUtils", () => { it("should return a mapping of regions to the nodes that are present in that region based on the provided storeIDs", () => { const stores = [1, 2, 3, 4, 5, 6, 7, 8, 9, 10] as StoreID[]; const clusterNodeIDToRegion = { - 1: "region1", - 2: "region2", - 3: "region1", - 4: "region2", - 5: "region1", - }; + 1: { region: "region1", stores: [1, 6] }, + 2: { region: "region2", stores: [2, 7] }, + 3: { region: "region1", stores: [3, 8] }, + 4: { region: "region2", stores: [4, 9] }, + 5: { region: "region1", stores: [5, 10] }, + } as Record; const clusterStoreIDToNodeID = { 1: 1, 2: 2, diff --git a/pkg/ui/workspaces/cluster-ui/src/util/nodeUtils.ts b/pkg/ui/workspaces/cluster-ui/src/util/nodeUtils.ts index c3b39ca09668..eb68e20c170e 100644 --- a/pkg/ui/workspaces/cluster-ui/src/util/nodeUtils.ts +++ b/pkg/ui/workspaces/cluster-ui/src/util/nodeUtils.ts @@ -3,6 +3,7 @@ // Use of this software is governed by the CockroachDB Software License // included in the /LICENSE file. +import { NodeStatus } from "src/api"; import { NodeID, StoreID } from "src/types/clusterTypes"; // mapStoreIDsToNodeRegions creates a mapping of regions @@ -10,7 +11,7 @@ import { NodeID, StoreID } from "src/types/clusterTypes"; // the provided storeIDs. export const mapStoreIDsToNodeRegions = ( stores: StoreID[], - clusterNodeIDToRegion: Record = {}, + clusterNodeIDToRegion: Record = {}, clusterStoreIDToNodeID: Record = {}, ): Record => { const nodes = stores.reduce((acc, storeID) => { @@ -20,7 +21,7 @@ export const mapStoreIDsToNodeRegions = ( const nodesByRegion: Record = {}; nodes.forEach(nodeID => { - const region = clusterNodeIDToRegion[nodeID]; + const region = clusterNodeIDToRegion[nodeID].region; if (!nodesByRegion[region]) { nodesByRegion[region] = []; } From 79849ee4c90453c6a213a08f1afd880067848855 Mon Sep 17 00:00:00 2001 From: Ricky Stewart Date: Tue, 29 Oct 2024 14:17:55 -0500 Subject: [PATCH 3/4] pebble_nightly_metamorphic: show sizes of artifacts We're seeing OOM's when trying to upload artifacts. Print out the sizes so we can validate our assumption the artifacts are very large. Epic: none Release note: None --- .../teamcity/cockroach/nightlies/pebble_nightly_metamorphic.sh | 2 ++ 1 file changed, 2 insertions(+) diff --git a/build/teamcity/cockroach/nightlies/pebble_nightly_metamorphic.sh b/build/teamcity/cockroach/nightlies/pebble_nightly_metamorphic.sh index 43fa1e6a191c..818428367c7a 100755 --- a/build/teamcity/cockroach/nightlies/pebble_nightly_metamorphic.sh +++ b/build/teamcity/cockroach/nightlies/pebble_nightly_metamorphic.sh @@ -36,3 +36,5 @@ echo "Pebble module Git SHA: $PEBBLE_SHA" BAZEL_SUPPORT_EXTRA_DOCKER_ARGS="-e BUILD_VCS_NUMBER=$PEBBLE_SHA -e GITHUB_API_TOKEN -e GITHUB_REPO -e TC_BUILDTYPE_ID -e TC_BUILD_BRANCH -e TC_BUILD_ID -e TC_SERVER_URL" \ run_bazel build/teamcity/cockroach/nightlies/pebble_nightly_metamorphic_impl.sh + +du -a -h artifacts | sort -hr From b5b76069914c511c5c3f7f6c0e29da7242bf5965 Mon Sep 17 00:00:00 2001 From: Marcus Gartner Date: Wed, 23 Oct 2024 12:16:09 +0100 Subject: [PATCH 4/4] opt: relax max stack size in test for stack overflow This commit relaxes the maximum Go stack size in bytes for a test added in #132701 from 50KB to 125KB. The very low max stack size was causing stack overflows to occur in unrelated functions, like parsing, in some nightly tests. I'm hoping that more than doubling this will eliminate the flakes. Fixes #133212 Release note: None --- pkg/sql/opt/xform/testdata/rules/select | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/sql/opt/xform/testdata/rules/select b/pkg/sql/opt/xform/testdata/rules/select index 968106f49268..4e5e57bee44c 100644 --- a/pkg/sql/opt/xform/testdata/rules/select +++ b/pkg/sql/opt/xform/testdata/rules/select @@ -2820,7 +2820,7 @@ CREATE TABLE t132669 ( # unnecessary recursion to trigger a stack overflow without having to make the # `IN` list below huge - triggering a stack overflow with Go's default max stack # size requires a list of ~1.6 million elements. -opt max-stack=50KB format=hide-all +opt max-stack=125KB format=hide-all SELECT * FROM t132669 WHERE a IN ( 1, 2, 3, 4, 5, 6, 7, 8, 9, 10,