cli/demo: various fixes #108566
cli/demo: various fixes #108566
Conversation
|
This change is |
knz
force-pushed
the
20230810-demo
branch
2 times, most recently
from
930cd37 to
cb6b12d
Compare
There was a problem hiding this comment.
very nice! thanks for discovering all this and improving it. my only comments are about comments + commit messages
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @knz and @yuzefovich)
-- commits line 2 at r1:
nit: consider mentioning a one sentence summary of what the fix was. most of the change seems to be just moving code from one function to another. was the problem the missing call in startServerInternal?
does this first commit also fix the "timeout" issue in #108331
pkg/cli/democluster/demo_cluster.go line 503 at r1 (raw file):
} func (c *transientCluster) startTenantService(
nit: this function could use a comment
Prior to this fix, secondary tenant servers were only started when the demo cluster was initialized. Subsequent node restarts did not also restart the secondary tenant server. This caused any operation (including `\demo ls`) that access the secondary tenant to fail. This patch ensures that the secondary tenant server is started every time a demo node is restarted. Release note: None
Release note: None
The `NumNodes()` method is meant to return the number of nodes. So it can vary as demo nodes are shut down or restarted. Given where it is called in `clisqlshell`, there is no user-visible change here. However, the intent is different to `NumServers` which does the other thing. Release note: None
The `GetLocality()` method takes a node ID and was incorrectly using that to index the server array directly. This is incorrect - the node IDs are properties of the server and a lookup must be performed to find the server. Release note (bug fix): A bug was fixed in `cockroach demo` whereby `\demo add` could sometimes crash with an error "`index out of range [...] with length ...`". This bug had been introduced in v19.x.
Release note (bug fix): A bug was fixed whereby the command `\demo decommission` in `cockroach demo` could sometime leave the demo cluster in a broken state. This bug had been introduced in v20.2. Release note (cli change): The command `\demo recommission` has been removed from `cockroach demo`. It had been obsolete and non-functional ever since v20.2.
Release note: None
There was a problem hiding this comment.
Reviewable status:
nit: consider mentioning a one sentence summary of what the fix was.
Done.
most of the change seems to be just moving code from one function to another. was the problem the missing call in
startServerInternal?
Yes.
does this first commit also fix the "timeout" issue in
acceptance: flake in TestDockerCLI_test_demo_node_cmds #108331
This was due to the broken decommission (\demo decommission needs a drain/shutdown before the decommission proper). Addressed in r5. I also added a release note there.
pkg/cli/democluster/demo_cluster.go line 503 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
nit: this function could use a comment
Done.
|
TFYR! bors r=rafiss |
|
This PR was included in a batch that was canceled, it will be automatically retried |
|
bors r- |
|
Canceled. |
|
bors r=rafiss |
|
Build succeeded: |
|
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from def8809 to blathers/backport-release-23.1-108566: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 23.1.x failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
backport here #108631 |
There was a problem hiding this comment.
Nice work of flushing all of these out! I only have a couple of nits.
Reviewed 6 of 6 files at r7, 1 of 1 files at r8, 1 of 1 files at r9, 1 of 1 files at r10, 4 of 4 files at r11, 2 of 2 files at r12, all commit messages.
Reviewable status:
pkg/cli/democluster/demo_cluster.go line 1014 at r8 (raw file):
if s.nodeID == nodeID { serverIdx = i break
nit: why not return here right away once match is found?
pkg/cli/democluster/demo_cluster.go line 1030 at r8 (raw file):
serverIdx := c.findServer(roachpb.NodeID(nodeID)) if serverIdx == -1 {
nit: consider extracting -1 as a unexported global constant since it's used in several spots.
There was a problem hiding this comment.
Reviewable status:
pkg/cli/democluster/demo_cluster.go line 1014 at r8 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
nit: why not return here right away once match is found?
Done in a later commit.
pkg/cli/democluster/demo_cluster.go line 1030 at r8 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
nit: consider extracting
-1as a unexported global constant since it's used in several spots.
Unneeded in the later commit.
See individual commits for details.
Informs (and will fix after backport) #96239.
Fixes #102257.
Fixes #108331.
Fixes #108563.
Fixes #107888.
Epic: CRDB-28893