From 68d9b53d7d7d437204f4f7a780f59d82c81c068d Mon Sep 17 00:00:00 2001 From: Xin Hao Zhang Date: Mon, 21 Mar 2022 15:25:11 -0400 Subject: [PATCH 1/2] ui: remove unnecessary uses of lodash in database pages Closes #68820 This commit removes the ue of functions from the lodash function where regular JS can be used. Release note: None --- .../databaseDetailsPage.stories.tsx | 62 +++++++++---------- .../databaseDetailsPage.tsx | 12 ++-- .../databaseTablePage.stories.tsx | 17 +++-- .../databaseTablePage/databaseTablePage.tsx | 3 +- .../databasesPage/databasesPage.stories.tsx | 2 +- .../src/databasesPage/databasesPage.tsx | 5 +- 6 files changed, 47 insertions(+), 54 deletions(-) diff --git a/pkg/ui/workspaces/cluster-ui/src/databaseDetailsPage/databaseDetailsPage.stories.tsx b/pkg/ui/workspaces/cluster-ui/src/databaseDetailsPage/databaseDetailsPage.stories.tsx index c00140b4b5f9..7804bed093b5 100644 --- a/pkg/ui/workspaces/cluster-ui/src/databaseDetailsPage/databaseDetailsPage.stories.tsx +++ b/pkg/ui/workspaces/cluster-ui/src/databaseDetailsPage/databaseDetailsPage.stories.tsx @@ -20,6 +20,7 @@ import { } from "src/storybook/fixtures"; import { DatabaseDetailsPage, + DatabaseDetailsPageDataTable, DatabaseDetailsPageProps, ViewMode, } from "./databaseDetailsPage"; @@ -86,41 +87,40 @@ const withoutData: DatabaseDetailsPageProps = { }, }; +function createTable(): DatabaseDetailsPageDataTable { + const roles = _.uniq(new Array(_.random(1, 3)).map(() => randomRole())); + const grants = _.uniq( + new Array(_.random(1, 5)).map(() => randomTablePrivilege()), + ); + + return { + name: randomName(), + details: { + loading: false, + loaded: true, + columnCount: _.random(5, 42), + indexCount: _.random(1, 6), + userCount: roles.length, + roles: roles, + grants: grants, + statsLastUpdated: moment("0001-01-01T00:00:00Z"), + }, + stats: { + loading: false, + loaded: true, + replicationSizeInBytes: _.random(1000.0) * 1024 ** _.random(1, 2), + rangeCount: _.random(50, 500), + nodesByRegionString: + "gcp-europe-west1(n8), gcp-us-east1(n1), gcp-us-west1(n6)", + }, + }; +} + const withData: DatabaseDetailsPageProps = { loading: false, loaded: true, name: randomName(), - tables: _.map(Array(42), _item => { - const roles = _.uniq( - _.map(new Array(_.random(1, 3)), _item => randomRole()), - ); - const grants = _.uniq( - _.map(new Array(_.random(1, 5)), _item => randomTablePrivilege()), - ); - - return { - name: randomName(), - details: { - loading: false, - loaded: true, - columnCount: _.random(5, 42), - indexCount: _.random(1, 6), - userCount: roles.length, - roles: roles, - grants: grants, - statsLastUpdated: moment("0001-01-01T00:00:00Z"), - }, - showNodeRegionsColumn: true, - stats: { - loading: false, - loaded: true, - replicationSizeInBytes: _.random(1000.0) * 1024 ** _.random(1, 2), - rangeCount: _.random(50, 500), - nodesByRegionString: - "gcp-europe-west1(n8), gcp-us-east1(n1), gcp-us-west1(n6)", - }, - }; - }), + tables: [createTable()], viewMode: ViewMode.Tables, sortSettingTables: { ascending: false, diff --git a/pkg/ui/workspaces/cluster-ui/src/databaseDetailsPage/databaseDetailsPage.tsx b/pkg/ui/workspaces/cluster-ui/src/databaseDetailsPage/databaseDetailsPage.tsx index 1ddde24a6c83..16bc3876ffe5 100644 --- a/pkg/ui/workspaces/cluster-ui/src/databaseDetailsPage/databaseDetailsPage.tsx +++ b/pkg/ui/workspaces/cluster-ui/src/databaseDetailsPage/databaseDetailsPage.tsx @@ -12,8 +12,6 @@ import React from "react"; import { Link, RouteComponentProps } from "react-router-dom"; import { Tooltip } from "antd"; import classNames from "classnames/bind"; -import _ from "lodash"; - import { Breadcrumbs } from "src/breadcrumbs"; import { Dropdown, DropdownOption } from "src/dropdown"; import { CaretRight } from "src/icon/caretRight"; @@ -203,7 +201,7 @@ export class DatabaseDetailsPage extends React.Component< return this.props.refreshDatabaseDetails(this.props.name); } - _.forEach(this.props.tables, table => { + this.props.tables.forEach(table => { if (!table.details.loaded && !table.details.loading) { return this.props.refreshTableDetails(this.props.name, table.name); } @@ -417,8 +415,8 @@ export class DatabaseDetailsPage extends React.Component< Roles ), - cell: table => _.join(table.details.roles, ", "), - sort: table => _.join(table.details.roles, ", "), + cell: table => table.details.roles.join(", "), + sort: table => table.details.roles.join(", "), className: cx("database-table__col-roles"), name: "roles", }, @@ -428,8 +426,8 @@ export class DatabaseDetailsPage extends React.Component< Grants ), - cell: table => _.join(table.details.grants, ", "), - sort: table => _.join(table.details.grants, ", "), + cell: table => table.details.grants.join(", "), + sort: table => table.details.grants.join(", "), className: cx("database-table__col-grants"), name: "grants", }, diff --git a/pkg/ui/workspaces/cluster-ui/src/databaseTablePage/databaseTablePage.stories.tsx b/pkg/ui/workspaces/cluster-ui/src/databaseTablePage/databaseTablePage.stories.tsx index bb444ba90f9d..5922cb4fa92a 100644 --- a/pkg/ui/workspaces/cluster-ui/src/databaseTablePage/databaseTablePage.stories.tsx +++ b/pkg/ui/workspaces/cluster-ui/src/databaseTablePage/databaseTablePage.stories.tsx @@ -10,7 +10,6 @@ import React from "react"; import { storiesOf } from "@storybook/react"; -import _ from "lodash"; import { withBackground, withRouterProvider } from "src/storybook/decorators"; import { @@ -84,15 +83,13 @@ const withData: DatabaseTablePageProps = { ) `, replicaCount: 7, - indexNames: _.map(Array(3), randomName), - grants: _.uniq( - _.map(Array(12), () => { - return { - user: randomRole(), - privilege: randomTablePrivilege(), - }; - }), - ), + indexNames: Array(3).map(randomName), + grants: [ + { + user: randomRole(), + privilege: randomTablePrivilege(), + }, + ], statsLastUpdated: moment("0001-01-01T00:00:00Z"), }, showNodeRegionsSection: true, diff --git a/pkg/ui/workspaces/cluster-ui/src/databaseTablePage/databaseTablePage.tsx b/pkg/ui/workspaces/cluster-ui/src/databaseTablePage/databaseTablePage.tsx index 5c8cf2d89c23..37b703465815 100644 --- a/pkg/ui/workspaces/cluster-ui/src/databaseTablePage/databaseTablePage.tsx +++ b/pkg/ui/workspaces/cluster-ui/src/databaseTablePage/databaseTablePage.tsx @@ -12,7 +12,6 @@ import React from "react"; import { Col, Row, Tabs } from "antd"; import { RouteComponentProps } from "react-router-dom"; import classNames from "classnames/bind"; -import _ from "lodash"; import { Tooltip } from "antd"; import { Heading } from "@cockroachlabs/ui-components"; @@ -421,7 +420,7 @@ export class DatabaseTablePage extends React.Component< /> diff --git a/pkg/ui/workspaces/cluster-ui/src/databasesPage/databasesPage.stories.tsx b/pkg/ui/workspaces/cluster-ui/src/databasesPage/databasesPage.stories.tsx index 5107bbb00052..a8d94beb51cd 100644 --- a/pkg/ui/workspaces/cluster-ui/src/databasesPage/databasesPage.stories.tsx +++ b/pkg/ui/workspaces/cluster-ui/src/databasesPage/databasesPage.stories.tsx @@ -76,7 +76,7 @@ const withData: DatabasesPageProps = { ascending: false, columnTitle: "name", }, - databases: _.map(Array(42), _item => { + databases: Array(42).map(() => { return { loading: false, loaded: true, diff --git a/pkg/ui/workspaces/cluster-ui/src/databasesPage/databasesPage.tsx b/pkg/ui/workspaces/cluster-ui/src/databasesPage/databasesPage.tsx index ba9ee87f54a0..9564f00ad541 100644 --- a/pkg/ui/workspaces/cluster-ui/src/databasesPage/databasesPage.tsx +++ b/pkg/ui/workspaces/cluster-ui/src/databasesPage/databasesPage.tsx @@ -12,7 +12,6 @@ import React from "react"; import { Link, RouteComponentProps } from "react-router-dom"; import { Tooltip } from "antd"; import classNames from "classnames/bind"; -import _ from "lodash"; import { Anchor } from "src/anchor"; import { StackIcon } from "src/icon/stackIcon"; @@ -175,12 +174,12 @@ export class DatabasesPage extends React.Component< return this.props.refreshDatabases(); } - _.forEach(this.props.databases, database => { + this.props.databases.forEach(database => { if (!database.loaded && !database.loading) { return this.props.refreshDatabaseDetails(database.name); } - _.forEach(database.missingTables, table => { + database.missingTables.forEach(table => { if (!table.loading) { return this.props.refreshTableStats(database.name, table.name); } From 825a4ee8889cc9f777f98c202ae1877d5989c6cd Mon Sep 17 00:00:00 2001 From: Tobias Grieger Date: Mon, 21 Mar 2022 15:05:11 +0100 Subject: [PATCH 2/2] kvserver: don't use caller's context for ProposalData Previously, for local proposals, we were threading the caller's context down into Raft. This has caused a few bugs related to context cancellation sensitivity - the execution of a raft command should not be impacted by whether or not the client's context has already finished. This commit derives the proposal context from the Replica's annotated context, using a tracing span derived from the client context. That way, the derived context is not cancelable, but contains the same tracing Span (thus providing similar utility) as before. Notably, the trace span should also already contain the log tags present in the client's context. The tracing has coverage through (at least) a few tests that make assertions about trace events from below-raft showing up in a trace collected by the client, which I verified by omitting the Span: ``` TestRaftSSTableSideloadingProposal TestReplicaAbandonProposal TestReplicaRetryRaftProposal TestReplicaBurstPendingCommandsAndRepropose TestReplicaReproposalWithNewLeaseIndexError TestFailureToProcessCommandClearsLocalResult TestDBAddSSTable ``` See #75656. Release note: None --- pkg/kv/kvserver/replica_application_cmd.go | 4 +--- .../kvserver/replica_application_decoder.go | 19 ++++++++++++++++++- pkg/kv/kvserver/replica_proposal.go | 2 +- 3 files changed, 20 insertions(+), 5 deletions(-) diff --git a/pkg/kv/kvserver/replica_application_cmd.go b/pkg/kv/kvserver/replica_application_cmd.go index 8d738a77ba4e..7202f2d7a8d1 100644 --- a/pkg/kv/kvserver/replica_application_cmd.go +++ b/pkg/kv/kvserver/replica_application_cmd.go @@ -51,9 +51,7 @@ type replicatedCmd struct { // Replica's proposal map. proposal *ProposalData - // ctx is a context that follows from the proposal's context if it was - // proposed locally. Otherwise, it will follow from the context passed to - // ApplyCommittedEntries. + // ctx is a non-cancelable context used to apply the command. ctx context.Context // sp is the tracing span corresponding to ctx. It is closed in // finishTracingSpan. This span "follows from" the proposer's span (even diff --git a/pkg/kv/kvserver/replica_application_decoder.go b/pkg/kv/kvserver/replica_application_decoder.go index 28b11ed7fc4a..4d21482d31cb 100644 --- a/pkg/kv/kvserver/replica_application_decoder.go +++ b/pkg/kv/kvserver/replica_application_decoder.go @@ -12,8 +12,10 @@ package kvserver import ( "context" + "fmt" "github.com/cockroachdb/cockroach/pkg/kv/kvserver/apply" + "github.com/cockroachdb/cockroach/pkg/util" "github.com/cockroachdb/cockroach/pkg/util/log" "github.com/cockroachdb/cockroach/pkg/util/quotapool" "github.com/cockroachdb/cockroach/pkg/util/tracing" @@ -144,8 +146,19 @@ func (d *replicaDecoder) createTracingSpans(ctx context.Context) { var it replicatedCmdBufSlice for it.init(&d.cmdBuf); it.Valid(); it.Next() { cmd := it.cur() + if cmd.IsLocal() { - cmd.ctx, cmd.sp = tracing.ChildSpan(cmd.proposal.ctx, opName) + // We intentionally don't propagate the client's cancellation policy (in + // cmd.ctx) onto the request. See #75656. + propCtx := ctx // raft scheduler's ctx + var propSp *tracing.Span + // If the client has a trace, put a child into propCtx. + if sp := tracing.SpanFromContext(cmd.proposal.ctx); sp != nil { + propCtx, propSp = sp.Tracer().StartSpanCtx( + propCtx, "local proposal", tracing.WithParent(sp), + ) + } + cmd.ctx, cmd.sp = propCtx, propSp } else if cmd.raftCmd.TraceData != nil { // The proposal isn't local, and trace data is available. Extract // the remote span and start a server-side span that follows from it. @@ -167,6 +180,10 @@ func (d *replicaDecoder) createTracingSpans(ctx context.Context) { } else { cmd.ctx, cmd.sp = tracing.ChildSpan(ctx, opName) } + + if util.RaceEnabled && cmd.ctx.Done() != nil { + panic(fmt.Sprintf("cancelable context observed during raft application: %+v", cmd)) + } } } diff --git a/pkg/kv/kvserver/replica_proposal.go b/pkg/kv/kvserver/replica_proposal.go index fa08b6fc227f..0a57827832a7 100644 --- a/pkg/kv/kvserver/replica_proposal.go +++ b/pkg/kv/kvserver/replica_proposal.go @@ -45,7 +45,7 @@ import ( // be returned to the caller. type ProposalData struct { // The caller's context, used for logging proposals, reproposals, message - // sends, and command application. In order to enable safely tracing events + // sends, but not command application. In order to enable safely tracing events // beneath, modifying this ctx field in *ProposalData requires holding the // raftMu. ctx context.Context