Skip to content

Commit

Permalink
startupmigrations: remove no-op SET CLUSTER SETTING version
Browse files Browse the repository at this point in the history
The migration writing the initial value of the cluster version setting
was also performing a `SET CLUSTER SETTING version = v`, where v was the
version is had just written in the settings table. This statement was a
no-op, because it amounted to an update from v to v [1].
This patch removes the statement.

[1] https://github.com/cockroachdb/cockroach/blob/f87728fee6d16e2a352ac76e7ae9225930032ce6/pkg/upgrade/upgrademanager/manager.go#L167-L171

Release note: None
Epic: None
  • Loading branch information
andreimatei committed Nov 16, 2022
1 parent 507721e commit b350828
Show file tree
Hide file tree
Showing 2 changed files with 2 additions and 24 deletions.
4 changes: 2 additions & 2 deletions pkg/server/admin_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1055,10 +1055,10 @@ func TestAdminAPIEvents(t *testing.T) {
{"create_database", false, 0, false, 3},
{"drop_table", false, 0, false, 2},
{"create_table", false, 0, false, 3},
{"set_cluster_setting", false, 0, false, 4},
{"set_cluster_setting", false, 0, false, 3},
// We use limit=true with no limit here because otherwise the
// expCount will mess up the expected total count below.
{"set_cluster_setting", true, 0, true, 4},
{"set_cluster_setting", true, 0, true, 3},
{"create_table", true, 0, false, 3},
{"create_table", true, -1, false, 3},
{"create_table", true, 2, false, 2},
Expand Down
22 changes: 0 additions & 22 deletions pkg/startupmigrations/migrations.go
Original file line number Diff line number Diff line change
Expand Up @@ -770,28 +770,6 @@ func populateVersionSetting(ctx context.Context, r runner) error {
}
}

// NB: We have to run with retry here due to the following "race" condition:
// - We're attempting to the set the cluster version at startup.
// - Setting the cluster version requires all nodes to be up and running, in
// order to push out all relevant version gates.
// - This list of "all nodes" is gathered by looking at all the liveness
// records in KV.
// - When starting a multi-node cluster all at once, nodes other than the
// one being bootstrapped join the cluster using the join RPC.
// - The join RPC results in the creation of a liveness record for the
// joining node, except it starts off in an expired state (leaving it to
// the joining node to heartbeat it for the very first time).
//
// Attempting to set the cluster version at startup, while there also may be
// other nodes trying to join, could then result in failures where the
// migration infrastructure find expired liveness records and gives up. To
// that end we'll simply retry, expecting the joining nodes to "come live"
// before long.
if err := r.execAsRootWithRetry(
ctx, "set-setting", "SET CLUSTER SETTING version = $1", v.String(),
); err != nil {
return err
}
return nil
}

Expand Down

0 comments on commit b350828

Please sign in to comment.