You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Dinesh Bhat (Code Review)" <ge...@cloudera.org> on 2016/09/16 17:51:50 UTC
[kudu-CR] cli tool: add -v to 'kudu table list'
Hello Dan Burkert, Adar Dembo,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/4440
to review the following change.
Change subject: cli tool: add -v to 'kudu table list'
......................................................................
cli tool: add -v to 'kudu table list'
I noticed that given a tablet or replica_uuid we need to execute
multiple nested commands and also need to corelate tablets and
their replica uuids and their relation to tables. Added a verbose
flag to 'kudu table list' to make this simpler.
This also incorporates few explicit object identifiers on local_replica
outputs, a feedback given by few support folks during training.
Change-Id: Ic8f8e0dfb8e7ba9f67d5926199a9b831351585a7
---
M src/kudu/tools/tool_action_common.cc
M src/kudu/tools/tool_action_local_replica.cc
M src/kudu/tools/tool_action_table.cc
3 files changed, 33 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/40/4440/1
--
To view, visit http://gerrit.cloudera.org:8080/4440
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic8f8e0dfb8e7ba9f67d5926199a9b831351585a7
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
[kudu-CR] cli tool: List all tablets/replica uuids with 'kudu table list'
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: cli tool: List all tablets/replica_uuids with 'kudu table list'
......................................................................
Patch Set 2:
> Also on an ortho topic of making the output to machine-readable by
> keeping loglevels to only WARNING and above: I realized couple of
> ways to do that is to use big hammer FLAG_minloglevel = 2 or
> google::SetCommandLineOption(minloglevel, 2) from code , but that
> also means we are losing the access to all logs below WARNING. Does
> that mean it's a no-go ? Come to think of it, any script wants to
> consume this output, it could use 'kudu ... 2>/dev/null' which
> isn't a bad assumption.
Firstly, I think we should stifle _all_ LOG messages, not just INFO or below. In the event of a failed action, the Status should contain enough information to figure out what's going on, and if not, the action can be rerun with --debug (or whatever option name you choose).
Secondly, you can use google::GetCommandLineFlagInfoOrDie() to get a CommandLineFlagInfo for a given gflag. Inside that there's a is_default field which will tell you if the user has overridden the default value. So, if is_default is true, you can adjust minloglevel as you please. If not, leave it alone and let the user's value carry through.
--
To view, visit http://gerrit.cloudera.org:8080/4440
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8f8e0dfb8e7ba9f67d5926199a9b831351585a7
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] cli tool: List all tablets/replica uuids with 'kudu table list'
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has submitted this change and it was merged.
Change subject: cli tool: List all tablets/replica_uuids with 'kudu table list'
......................................................................
cli tool: List all tablets/replica_uuids with 'kudu table list'
I noticed that given a tablet or replica_uuid we need to execute
multiple nested commands and also need to correlate tablets and
their replica uuids and their relation to tables. Added a verbose
flag to 'kudu table list' to make this simpler.
This lists tablet/replica uuids irrespective of their states.
Change-Id: Ic8f8e0dfb8e7ba9f67d5926199a9b831351585a7
Reviewed-on: http://gerrit.cloudera.org:8080/4440
Tested-by: Kudu Jenkins
Reviewed-by: Dan Burkert <da...@cloudera.com>
---
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/tool_action_local_replica.cc
M src/kudu/tools/tool_action_table.cc
3 files changed, 89 insertions(+), 10 deletions(-)
Approvals:
Dan Burkert: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/4440
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ic8f8e0dfb8e7ba9f67d5926199a9b831351585a7
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] cli tool: List all tablets/replica uuids with 'kudu table list'
Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Adar Dembo, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4440
to look at the new patch set (#7).
Change subject: cli tool: List all tablets/replica_uuids with 'kudu table list'
......................................................................
cli tool: List all tablets/replica_uuids with 'kudu table list'
I noticed that given a tablet or replica_uuid we need to execute
multiple nested commands and also need to correlate tablets and
their replica uuids and their relation to tables. Added a verbose
flag to 'kudu table list' to make this simpler.
This lists tablet/replica uuids irrespective of their states.
Change-Id: Ic8f8e0dfb8e7ba9f67d5926199a9b831351585a7
---
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/tool_action_local_replica.cc
M src/kudu/tools/tool_action_table.cc
3 files changed, 89 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/40/4440/7
--
To view, visit http://gerrit.cloudera.org:8080/4440
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic8f8e0dfb8e7ba9f67d5926199a9b831351585a7
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] cli tool: List all tablets/replica uuids with 'kudu table list'
Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.
Dinesh Bhat has posted comments on this change.
Change subject: cli tool: List all tablets/replica_uuids with 'kudu table list'
......................................................................
Patch Set 4:
(1 comment)
TFTR Adar, updated the patch with new test added.
http://gerrit.cloudera.org:8080/#/c/4440/3/src/kudu/tools/kudu-admin-test.cc
File src/kudu/tools/kudu-admin-test.cc:
Line 192: }
> Sure, you could add another CreateTable() variant. Alternatively, you could
Thanks, since the tablet/tserver data were available in place, I tested their output too.
--
To view, visit http://gerrit.cloudera.org:8080/4440
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8f8e0dfb8e7ba9f67d5926199a9b831351585a7
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] cli tool: List all tablets/replica uuids with 'kudu table list'
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: cli tool: List all tablets/replica_uuids with 'kudu table list'
......................................................................
Patch Set 7:
Just looked at the output / formatting, but LGTM
--
To view, visit http://gerrit.cloudera.org:8080/4440
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8f8e0dfb8e7ba9f67d5926199a9b831351585a7
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] cli tool: List all tablets/replica uuids with 'kudu table list'
Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Adar Dembo, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4440
to look at the new patch set (#4).
Change subject: cli tool: List all tablets/replica_uuids with 'kudu table list'
......................................................................
cli tool: List all tablets/replica_uuids with 'kudu table list'
I noticed that given a tablet or replica_uuid we need to execute
multiple nested commands and also need to correlate tablets and
their replica uuids and their relation to tables. Added a verbose
flag to 'kudu table list' to make this simpler.
Change-Id: Ic8f8e0dfb8e7ba9f67d5926199a9b831351585a7
---
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/tool_action_local_replica.cc
M src/kudu/tools/tool_action_table.cc
3 files changed, 88 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/40/4440/4
--
To view, visit http://gerrit.cloudera.org:8080/4440
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic8f8e0dfb8e7ba9f67d5926199a9b831351585a7
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] cli tool: List all tablets/replica uuids with 'kudu table list'
Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Adar Dembo, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4440
to look at the new patch set (#5).
Change subject: cli tool: List all tablets/replica_uuids with 'kudu table list'
......................................................................
cli tool: List all tablets/replica_uuids with 'kudu table list'
I noticed that given a tablet or replica_uuid we need to execute
multiple nested commands and also need to correlate tablets and
their replica uuids and their relation to tables. Added a verbose
flag to 'kudu table list' to make this simpler.
Change-Id: Ic8f8e0dfb8e7ba9f67d5926199a9b831351585a7
---
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/tool_action_local_replica.cc
M src/kudu/tools/tool_action_table.cc
3 files changed, 88 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/40/4440/5
--
To view, visit http://gerrit.cloudera.org:8080/4440
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic8f8e0dfb8e7ba9f67d5926199a9b831351585a7
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] cli tool: List all tablets/replica uuids with 'kudu table list'
Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.
Dinesh Bhat has posted comments on this change.
Change subject: cli tool: List all tablets/replica_uuids with 'kudu table list'
......................................................................
Patch Set 2:
(2 comments)
> (2 comments)
>
> Would be nice to get a little coverage of the new path, especially
> since there was that bug in the first patch which didn't list all
> the tables. Could you add something to TestListTables in
> kudu-admin-test? We should test listing of multiple tables, and
> verify the new output if possible.
Thanks, yeah that alarmed a test for sure. I will upload a new patch with test.
Also on an ortho topic of making the output to machine-readable by keeping loglevels to only WARNING and above: I realized couple of ways to do that is to use big hammer FLAG_minloglevel = 2 or google::SetCommandLineOption(minloglevel, 2) from code , but that also means we are losing the access to all logs below WARNING. Does that mean it's a no-go ? Come to think of it, any script wants to consume this output, it could use 'kudu ... 2>/dev/null' which isn't a bad assumption.
http://gerrit.cloudera.org:8080/#/c/4440/2/src/kudu/tools/tool_action_local_replica.cc
File src/kudu/tools/tool_action_local_replica.cc:
Line 641: .AddOptionalParameter("dump_data")
> Nit: maintain alphabetical order.
Done
http://gerrit.cloudera.org:8080/#/c/4440/2/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:
PS2, Line 34: "Print list of tablets for the table along with replica uuids"
> Nit: how about "Include tablet and replica UUIDs in the output"?
Done
--
To view, visit http://gerrit.cloudera.org:8080/4440
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8f8e0dfb8e7ba9f67d5926199a9b831351585a7
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] cli tool: List all tablets/replica uuids with 'kudu table list'
Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.
Dinesh Bhat has posted comments on this change.
Change subject: cli tool: List all tablets/replica_uuids with 'kudu table list'
......................................................................
Patch Set 2:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/4440/1//COMMIT_MSG
Commit Message:
PS1, Line 10: correlat
> correlate
Done
http://gerrit.cloudera.org:8080/#/c/4440/1/src/kudu/tools/tool_action_local_replica.cc
File src/kudu/tools/tool_action_local_replica.cc:
PS1, Line 67: false,
> Nit: don't need this breadcrumb. I only added it for timeout_ms because tha
Done
Line 178: unique_ptr<ConsensusMetadata> cmeta;
> I'd rather we didn't do this; it makes the output less machine-readable. Or
Alrighty, as discussed offline, removed them.
http://gerrit.cloudera.org:8080/#/c/4440/1/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:
PS1, Line 81: cout << tname << endl;
: if (!FLAGS_list_tablets) {
: continue;
: }
: client::sp::shared_ptr<KuduTable> client_table;
: RETURN_NOT_OK(client->OpenTable(tname,
> All this should happen after the verbose check, since its output isn't need
Done
PS1, Line 89: KuduScanTokenBuilder builder(client_table.get());
: RETURN_NOT_OK(builder.Build(&tokens));
:
> Won't this cause just the first table to be printed?
Good catch, looks like I was testing with one table too :)
PS1, Line 95: cout << "P" << (replica->is_leader() ? "(L) " : " ")
: << replica->ts().uuid() << " ";
> To avoid leaking too many Raft implementation details, let's annotate the
Done
Line 124: .AddOptionalParameter("list_tablets")
> Printing extra information during "kudu table list" is useful, but I'd pref
done, i pinged in hipchat before seeing this comment of yours, happy that we arrived at same conclusion. i replaced '--list-tablets' or suggest with any other choices.
--
To view, visit http://gerrit.cloudera.org:8080/4440
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8f8e0dfb8e7ba9f67d5926199a9b831351585a7
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] cli tool: List all tablets/replica uuids with 'kudu table list'
Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.
Dinesh Bhat has posted comments on this change.
Change subject: cli tool: List all tablets/replica_uuids with 'kudu table list'
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4440/3/src/kudu/tools/kudu-admin-test.cc
File src/kudu/tools/kudu-admin-test.cc:
Line 192: TEST_F(AdminCliTest, TestListTables) {
> No change here for multiple tables? Or in the new test?
Yeah, I want to make below test populated with mulitple tables. Do you have any examples/inputs how to create multiple tables without re-inventing wheel here ? The way I see current structure of TabletIntegrationTestBase, it seems to be written for single cluster/table scenario. Perhaps I can add another routine to that CreateTable(string table-name) ? It prolly also means that we also need additional instances of KuduTable and tablet_id_ to track.
--
To view, visit http://gerrit.cloudera.org:8080/4440
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8f8e0dfb8e7ba9f67d5926199a9b831351585a7
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] cli tool: List all tablets/replica uuids with 'kudu table list'
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: cli tool: List all tablets/replica_uuids with 'kudu table list'
......................................................................
Patch Set 7: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/4440
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8f8e0dfb8e7ba9f67d5926199a9b831351585a7
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] cli tool: List all tablets/replica uuids with 'kudu table list'
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: cli tool: List all tablets/replica_uuids with 'kudu table list'
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4440/3/src/kudu/tools/kudu-admin-test.cc
File src/kudu/tools/kudu-admin-test.cc:
Line 192: TEST_F(AdminCliTest, TestListTables) {
> Yeah, I want to make below test populated with mulitple tables. Do you have
Sure, you could add another CreateTable() variant. Alternatively, you could access client_ directly, create a new KuduTableCreator, and create your table.
I don't think you need to test its tablet/replica distribution, unless you really want to. In effect, this is just a quick test for the regression that was almost introduced, ensuring that more than one table is listed.
--
To view, visit http://gerrit.cloudera.org:8080/4440
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8f8e0dfb8e7ba9f67d5926199a9b831351585a7
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] cli tool: List all tablets/replica uuids with 'kudu table list'
Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Adar Dembo, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4440
to look at the new patch set (#6).
Change subject: cli tool: List all tablets/replica_uuids with 'kudu table list'
......................................................................
cli tool: List all tablets/replica_uuids with 'kudu table list'
I noticed that given a tablet or replica_uuid we need to execute
multiple nested commands and also need to correlate tablets and
their replica uuids and their relation to tables. Added a verbose
flag to 'kudu table list' to make this simpler.
Change-Id: Ic8f8e0dfb8e7ba9f67d5926199a9b831351585a7
---
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/tool_action_local_replica.cc
M src/kudu/tools/tool_action_table.cc
3 files changed, 89 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/40/4440/6
--
To view, visit http://gerrit.cloudera.org:8080/4440
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic8f8e0dfb8e7ba9f67d5926199a9b831351585a7
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] cli tool: List all tablets/replica uuids with 'kudu table list'
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: cli tool: List all tablets/replica_uuids with 'kudu table list'
......................................................................
Patch Set 2:
(2 comments)
Would be nice to get a little coverage of the new path, especially since there was that bug in the first patch which didn't list all the tables. Could you add something to TestListTables in kudu-admin-test? We should test listing of multiple tables, and verify the new output if possible.
http://gerrit.cloudera.org:8080/#/c/4440/2/src/kudu/tools/tool_action_local_replica.cc
File src/kudu/tools/tool_action_local_replica.cc:
Line 641: .AddOptionalParameter("dump_data")
Nit: maintain alphabetical order.
http://gerrit.cloudera.org:8080/#/c/4440/2/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:
PS2, Line 34: "Print list of tablets for the table along with replica uuids"
Nit: how about "Include tablet and replica UUIDs in the output"?
--
To view, visit http://gerrit.cloudera.org:8080/4440
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8f8e0dfb8e7ba9f67d5926199a9b831351585a7
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] cli tool: List all tablets/replica uuids with 'kudu table list'
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: cli tool: List all tablets/replica_uuids with 'kudu table list'
......................................................................
Patch Set 5: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/4440
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8f8e0dfb8e7ba9f67d5926199a9b831351585a7
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] cli tool: List all tablets/replica uuids with 'kudu table list'
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: cli tool: List all tablets/replica_uuids with 'kudu table list'
......................................................................
Patch Set 6: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/4440
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8f8e0dfb8e7ba9f67d5926199a9b831351585a7
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] cli tool: add -v to 'kudu table list'
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: cli tool: add -v to 'kudu table list'
......................................................................
Patch Set 1:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/4440/1//COMMIT_MSG
Commit Message:
PS1, Line 10: corelate
correlate
http://gerrit.cloudera.org:8080/#/c/4440/1/src/kudu/tools/tool_action_local_replica.cc
File src/kudu/tools/tool_action_local_replica.cc:
PS1, Line 67: // defined in tool_action_common
Nit: don't need this breadcrumb. I only added it for timeout_ms because that was defined in ksck, which is a separate "module" from the CLI tool.
Line 178: cout << "List of replica uuids for tablet " << tablet_id << ":" << endl;
I'd rather we didn't do this; it makes the output less machine-readable. Or at least introduce a switch to control whether the output is human-readable or machine-readable.
http://gerrit.cloudera.org:8080/#/c/4440/1/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:
Line 21: #include <iterator>
Don't need this anymore.
PS1, Line 34: // defined in tool_action_common
Don't need this.
PS1, Line 81: client::sp::shared_ptr<KuduTable> client_table;
: RETURN_NOT_OK(client->OpenTable(tname, &client_table));
: vector<KuduScanToken*> tokens;
: ElementDeleter deleter(&tokens);
: KuduScanTokenBuilder builder(client_table.get());
: RETURN_NOT_OK(builder.Build(&tokens));
All this should happen after the verbose check, since its output isn't needed if !verbose.
PS1, Line 89: if (!FLAGS_verbose) {
: return Status::OK();
: }
Won't this cause just the first table to be printed?
PS1, Line 95: cout << "P" << (replica->is_leader() ? "(L) " : "(F) ")
: << replica->ts().uuid() << " ";
To avoid leaking too many Raft implementation details, let's annotate the leader replica with (L) but let's not annotate the followers at all.
Line 124: .AddOptionalParameter("verbose")
Printing extra information during "kudu table list" is useful, but I'd prefer if we didn't use 'verbose' for it. I was thinking we'd use 'verbose' to control glog output. Right now if we want users to see important output we have to redirect stderr to /dev/null, because there's enough INFO-level glog output that it muddies up the stdout output. In the future, we can hide all of that by default and use 'verbose' to turn it on when someone is debugging.
So let's replace this with something targeted for list_tables, such as "include_replicas" or some such.
--
To view, visit http://gerrit.cloudera.org:8080/4440
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8f8e0dfb8e7ba9f67d5926199a9b831351585a7
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] cli tool: List all tablets/replica uuids with 'kudu table list'
Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.
Dinesh Bhat has posted comments on this change.
Change subject: cli tool: List all tablets/replica_uuids with 'kudu table list'
......................................................................
Patch Set 3:
> > Also on an ortho topic of making the output to machine-readable
> by
> > keeping loglevels to only WARNING and above: I realized couple of
> > ways to do that is to use big hammer FLAG_minloglevel = 2 or
> > google::SetCommandLineOption(minloglevel, 2) from code , but that
> > also means we are losing the access to all logs below WARNING.
> Does
> > that mean it's a no-go ? Come to think of it, any script wants to
> > consume this output, it could use 'kudu ... 2>/dev/null' which
> > isn't a bad assumption.
>
> Firstly, I think we should stifle _all_ LOG messages, not just INFO
> or below. In the event of a failed action, the Status should
> contain enough information to figure out what's going on, and if
> not, the action can be rerun with --debug (or whatever option name
> you choose).
>
> Secondly, you can use google::GetCommandLineFlagInfoOrDie() to get
> a CommandLineFlagInfo for a given gflag. Inside that there's a
> is_default field which will tell you if the user has overridden the
> default value. So, if is_default is true, you can adjust
> minloglevel as you please. If not, leave it alone and let the
> user's value carry through.
Thanks Adar, updated both patches, please re-review. I have changed the diffs as per the second suggestion above since there is still some value add in keeping a backdoor control to dump all loglevel messages.
PS: The asan build seems to be failing in some unrelated place, pls ignore build failure for now.
--
To view, visit http://gerrit.cloudera.org:8080/4440
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8f8e0dfb8e7ba9f67d5926199a9b831351585a7
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] cli tool: List all tablets/replica uuids with 'kudu table list'
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: cli tool: List all tablets/replica_uuids with 'kudu table list'
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4440/3/src/kudu/tools/kudu-admin-test.cc
File src/kudu/tools/kudu-admin-test.cc:
Line 192: TEST_F(AdminCliTest, TestListTables) {
No change here for multiple tables? Or in the new test?
--
To view, visit http://gerrit.cloudera.org:8080/4440
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8f8e0dfb8e7ba9f67d5926199a9b831351585a7
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] cli tool: List all tablets/replica uuids with 'kudu table list'
Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Adar Dembo, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4440
to look at the new patch set (#2).
Change subject: cli tool: List all tablets/replica_uuids with 'kudu table list'
......................................................................
cli tool: List all tablets/replica_uuids with 'kudu table list'
I noticed that given a tablet or replica_uuid we need to execute
multiple nested commands and also need to correlate tablets and
their replica uuids and their relation to tables. Added a verbose
flag to 'kudu table list' to make this simpler.
Change-Id: Ic8f8e0dfb8e7ba9f67d5926199a9b831351585a7
---
M src/kudu/tools/tool_action_local_replica.cc
M src/kudu/tools/tool_action_table.cc
2 files changed, 38 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/40/4440/2
--
To view, visit http://gerrit.cloudera.org:8080/4440
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic8f8e0dfb8e7ba9f67d5926199a9b831351585a7
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] cli tool: List all tablets/replica uuids with 'kudu table list'
Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.
Dinesh Bhat has posted comments on this change.
Change subject: cli tool: List all tablets/replica_uuids with 'kudu table list'
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4440/4/src/kudu/tools/kudu-admin-test.cc
File src/kudu/tools/kudu-admin-test.cc:
PS4, Line 251: ASSERT_STR_CONTAINS(stdout, kTableId);
: ASSERT_STR_CONTAINS(stdout, kAnotherT
> Is there a guarantee that the two tables will be printed in this order?
Hmmm, I looked at my test output and another cluster output where I had 4 tables, and they seem to be ordered alphabetically. However, after looking at ListTables/CreateTable service handlers in catalog_manager they are using std::unordered_map which doesn't guarantee anything about ordering of the keys in the map. For safety purposes, I am removing this assumption from code. Thanks for catching that !!
--
To view, visit http://gerrit.cloudera.org:8080/4440
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8f8e0dfb8e7ba9f67d5926199a9b831351585a7
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] cli tool: List all tablets/replica uuids with 'kudu table list'
Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Adar Dembo, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4440
to look at the new patch set (#3).
Change subject: cli tool: List all tablets/replica_uuids with 'kudu table list'
......................................................................
cli tool: List all tablets/replica_uuids with 'kudu table list'
I noticed that given a tablet or replica_uuid we need to execute
multiple nested commands and also need to correlate tablets and
their replica uuids and their relation to tables. Added a verbose
flag to 'kudu table list' to make this simpler.
Change-Id: Ic8f8e0dfb8e7ba9f67d5926199a9b831351585a7
---
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/tool_action_local_replica.cc
M src/kudu/tools/tool_action_table.cc
3 files changed, 66 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/40/4440/3
--
To view, visit http://gerrit.cloudera.org:8080/4440
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic8f8e0dfb8e7ba9f67d5926199a9b831351585a7
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] cli tool: List all tablets/replica uuids with 'kudu table list'
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: cli tool: List all tablets/replica_uuids with 'kudu table list'
......................................................................
Patch Set 4: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4440/4/src/kudu/tools/kudu-admin-test.cc
File src/kudu/tools/kudu-admin-test.cc:
PS4, Line 251: ASSERT_EQ(kAnotherTableId, stdout_lines[0]);
: ASSERT_EQ(kTableId, stdout_lines[2]);
Is there a guarantee that the two tables will be printed in this order?
--
To view, visit http://gerrit.cloudera.org:8080/4440
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8f8e0dfb8e7ba9f67d5926199a9b831351585a7
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] cli tool: List all tablets/replica uuids with 'kudu table list'
Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.
Dinesh Bhat has posted comments on this change.
Change subject: cli tool: List all tablets/replica_uuids with 'kudu table list'
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4440/3/src/kudu/tools/kudu-admin-test.cc
File src/kudu/tools/kudu-admin-test.cc:
Line 192: TEST_F(AdminCliTest, TestListTables) {
> Yeah, I want to make below test populated with mulitple tables. Do you have
Never mind, I was able to create another table using same client handle and schema, I will try to wrap this up with mutliple table output.
--
To view, visit http://gerrit.cloudera.org:8080/4440
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8f8e0dfb8e7ba9f67d5926199a9b831351585a7
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes