-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
cli: use zone configs to disable replication #39492
Conversation
pkg/cli/demo.go
Outdated
// Set up the default zone configuration. We are using an in-memory store | ||
// so we really want to disable replication. | ||
// Note: we use the raw URL without a database (Path) | ||
// as it is not creaed yet. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo
@@ -416,18 +416,18 @@ func runStartSingleNode(cmd *cobra.Command, args []string) error { | |||
// Now actually set the flag as changed so that the start code | |||
// doesn't warn that it was not set. | |||
joinFlag.Changed = true | |||
return runStart(cmd, args) | |||
return runStart(cmd, args, true /*disableReplication*/) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if I start a non-single node three node cluster with nodes 2 and 3 pointing at node 1 (which itself joins to nobody), and then I restart node one as a single-node instance? Will it just work, error out, or erroneously overwrite my zone configs?
Regarding the second point, it only configures the zones if initialBoot == true as we discussed.
So no-op if the cluster is already initialized.
The PR is not complete btw I need to tweak some tests to further clarify this specific point.
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
|
81c5595
to
49ac0fd
Compare
I have added tests that specifically check that the replication factor is not (re)set if the cluster is already initialized. RFAL. |
49ac0fd
to
0ff4756
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @bdarnell, @solongordon, and @tbg)
pkg/cli/demo.go, line 142 at r1 (raw file):
Previously, tbg (Tobias Grieger) wrote…
typo
Done.
pkg/cli/start.go, line 419 at r1 (raw file):
Previously, tbg (Tobias Grieger) wrote…
What if I start a non-single node three node cluster with nodes 2 and 3 pointing at node 1 (which itself joins to nobody), and then I restart node one as a single-node instance? Will it just work, error out, or erroneously overwrite my zone configs?
Added a comment to explain what's going on (and tests to verify)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @bdarnell, @knz, @solongordon, and @tbg)
pkg/cli/disable_replication.go, line 47 at r2 (raw file):
} log.Shout(ctx, log.Severity_INFO, "Replication was disabled for this cluster.\n"+
Single-node cockroach demo
shouldn't give this warning since you can't add nodes to it later (unless we're changing the default to 3 nodes, in which case I'm fine with this message for the uncommon case of cockroach demo --nodes=1
)
pkg/cli/interactive_tests/test_disable_replication.tcl, line 23 at r2 (raw file):
end_test start_test "Check that it remains possible to reset the replication factor"
We should have a that actually does add two more nodes to the formerly-single-node cluster.
That is incorrect, it works perfectly fine, see #39505 for an example application.
will do |
Interesting. But just because it's possible to do that doesn't mean it warrants a prominent message when starting up a demo that is almost certainly going to remain single-node. |
As established in cockroachdb#39382 it is not safe to modify the global default zone config objects to non-standard values. This commit changes `start-single-node` and `demo` to use zone configs via SQL instead. Also as requested by @piyush-singh it now informs the user the replication was disabled, for example: ``` * * INFO: Replication was disabled for this cluster. * When/if adding nodes in the future, update zone configurations to increase the replication factor. * ``` Release note: None
0ff4756
to
b233dba
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @bdarnell, @solongordon, and @tbg)
pkg/cli/disable_replication.go, line 47 at r2 (raw file):
Previously, bdarnell (Ben Darnell) wrote…
Single-node
cockroach demo
shouldn't give this warning since you can't add nodes to it later (unless we're changing the default to 3 nodes, in which case I'm fine with this message for the uncommon case ofcockroach demo --nodes=1
)
Done.
pkg/cli/interactive_tests/test_disable_replication.tcl, line 23 at r2 (raw file):
Previously, bdarnell (Ben Darnell) wrote…
We should have a that actually does add two more nodes to the formerly-single-node cluster.
Done.
TFYR! bors r+ |
39492: cli: use zone configs to disable replication r=knz a=knz Fixes #39379. As established in #39382 it is not safe to modify the global default zone config objects to non-standard values. This commit changes `start-single-node` and `demo` to use zone configs via SQL instead. Also as requested by @piyush-singh it now informs the user the replication was disabled, for example: ``` * * INFO: Replication was disabled for this cluster. * When/if adding nodes in the future, update zone configurations to increase the replication factor. * ``` Release note: None Co-authored-by: Raphael 'kena' Poss <[email protected]>
Build succeeded |
Fixes #39379.
As established in #39382 it is not safe to modify the global
default zone config objects to non-standard values.
This commit changes
start-single-node
anddemo
to usezone configs via SQL instead.
Also as requested by @piyush-singh it now informs the user
the replication was disabled, for example:
Release note: None