You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2020/03/01 06:52:38 UTC
[kudu-CR] ksck: display quiecing-related info
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15323 )
Change subject: ksck: display quiecing-related info
......................................................................
Patch Set 1:
(5 comments)
The test failures seem relevant to these changes.
http://gerrit.cloudera.org:8080/#/c/15323/1/src/kudu/integration-tests/tablet_server_quiescing-itest.cc
File src/kudu/integration-tests/tablet_server_quiescing-itest.cc:
http://gerrit.cloudera.org:8080/#/c/15323/1/src/kudu/integration-tests/tablet_server_quiescing-itest.cc@398
PS1, Line 398: " Quiescing | Tablet leader count | Active scanner count\n"
: "-----------+---------------------+----------------------\n"
: " true | 1 | 0");
: ASSERT_TRUE(ts->server()->quiescing());
:
: // Same with ksck.
: ASSERT_OK(RunKuduTool({ "cluster", "ksck", master_addr }, &stdout));
: ASSERT_STR_MATCHES(stdout,
: ".* Quiescing | Tablet Leaders | Active Scanners\n"
: ".* ----------+----------------+----------------\n"
: ".* true | 1 | 0");
nit: maybe, make the names of the corresponding columns match for both 'tserver quiesce' and 'cluster ksck'?
http://gerrit.cloudera.org:8080/#/c/15323/1/src/kudu/integration-tests/tablet_server_quiescing-itest.cc@433
PS1, Line 433: --noquiescing_info
Instead, maybe output the quiescing information only if --quiescing_info flag is specified?
http://gerrit.cloudera.org:8080/#/c/15323/1/src/kudu/tools/ksck_remote.cc
File src/kudu/tools/ksck_remote.cc:
http://gerrit.cloudera.org:8080/#/c/15323/1/src/kudu/tools/ksck_remote.cc@81
PS1, Line 81: true
Do we really want to include the information into ksck by default? Given the presence of a dedicated sub-command for quiescing and thinking about backwards compatibility, I would expect it is set to 'false' by default, no?
http://gerrit.cloudera.org:8080/#/c/15323/1/src/kudu/tools/ksck_remote.cc@82
PS1, Line 82: to check
What does 'check' means here? Simply 'display'?
http://gerrit.cloudera.org:8080/#/c/15323/1/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:
http://gerrit.cloudera.org:8080/#/c/15323/1/src/kudu/tserver/tablet_service.cc@1020
PS1, Line 1020: default:
What about other already supported features?
--
To view, visit http://gerrit.cloudera.org:8080/15323
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibdc650eb3ee30e8993330f2cbd389076ea2bad49
Gerrit-Change-Number: 15323
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Sun, 01 Mar 2020 06:52:38 +0000
Gerrit-HasComments: Yes