You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org> on 2022/07/27 22:26:44 UTC

[kudu-CR] [tools] KUDU-2671 Update the kudu table describe tool

Abhishek Chennaka has uploaded this change for review. ( http://gerrit.cloudera.org:8080/18794


Change subject: [tools] KUDU-2671 Update the kudu table describe tool
......................................................................

[tools] KUDU-2671 Update the kudu table describe tool

This patch updates the kudu table describe tool to output ranges
which have a custom hash schema with the corresponding hash schema.
If a partition has multiple custom hash dimensions they are
separated by a space.

Change-Id: I9fa2eb965051a5f63d1c482e6fd43ff654ec6364
---
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/tool_action_table.cc
2 files changed, 62 insertions(+), 3 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/18794/1
-- 
To view, visit http://gerrit.cloudera.org:8080/18794
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I9fa2eb965051a5f63d1c482e6fd43ff654ec6364
Gerrit-Change-Number: 18794
Gerrit-PatchSet: 1
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>

[kudu-CR] [tools] KUDU-2671 Update the kudu table describe tool

Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/18794 )

Change subject: [tools] KUDU-2671 Update the kudu table describe tool
......................................................................


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/18794/1/src/kudu/tools/kudu-admin-test.cc
File src/kudu/tools/kudu-admin-test.cc:

http://gerrit.cloudera.org:8080/#/c/18794/1/src/kudu/tools/kudu-admin-test.cc@2114
PS1, Line 2114:         
> here and below: wrong indent
Done


http://gerrit.cloudera.org:8080/#/c/18794/1/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:

http://gerrit.cloudera.org:8080/#/c/18794/1/src/kudu/tools/tool_action_table.cc@291
PS1, Line 291: const char* const kHashSchemaJSONArg = "create_table_json";
> Is this needed?
No, it shouldn't be there. Thanks for catching it.



-- 
To view, visit http://gerrit.cloudera.org:8080/18794
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9fa2eb965051a5f63d1c482e6fd43ff654ec6364
Gerrit-Change-Number: 18794
Gerrit-PatchSet: 1
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 27 Jul 2022 23:17:05 +0000
Gerrit-HasComments: Yes

[kudu-CR] [tools] KUDU-2671 Update the kudu table describe tool

Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/18794 )

Change subject: [tools] KUDU-2671 Update the kudu table describe tool
......................................................................


Patch Set 3:

As discussed we want to have the output of the kudu table describe close to the current schema/partition schema of the table by default from a user perspective.


-- 
To view, visit http://gerrit.cloudera.org:8080/18794
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9fa2eb965051a5f63d1c482e6fd43ff654ec6364
Gerrit-Change-Number: 18794
Gerrit-PatchSet: 3
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 28 Jul 2022 20:52:24 +0000
Gerrit-HasComments: No

[kudu-CR] [tools] KUDU-2671 Update the kudu table describe tool

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18794 )

Change subject: [tools] KUDU-2671 Update the kudu table describe tool
......................................................................


Patch Set 3: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/18794/3/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:

http://gerrit.cloudera.org:8080/#/c/18794/3/src/kudu/tools/tool_action_table.cc@438
PS3, Line 438:         partition_schema.RangeWithCustomHashPartitionDebugString(partition.begin().range_key(),
             :                                                                  partition.end().range_key(),
             :                                                                  schema_internal);
After comparing this to the output in the UI of the embedded web server, I thought that an alternative way of doing this might be printing out hash schema information on every range if some extra flag is provided.

Just trying to find the best path forward.    What do you think?

This also looks good enough to me, though.



-- 
To view, visit http://gerrit.cloudera.org:8080/18794
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9fa2eb965051a5f63d1c482e6fd43ff654ec6364
Gerrit-Change-Number: 18794
Gerrit-PatchSet: 3
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 28 Jul 2022 18:54:48 +0000
Gerrit-HasComments: Yes

[kudu-CR] [tools] KUDU-2671 Update the kudu table describe tool

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18794 )

Change subject: [tools] KUDU-2671 Update the kudu table describe tool
......................................................................


Patch Set 2:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/18794/2/src/kudu/tools/kudu-admin-test.cc
File src/kudu/tools/kudu-admin-test.cc:

http://gerrit.cloudera.org:8080/#/c/18794/2/src/kudu/tools/kudu-admin-test.cc@2103
PS2, Line 2103: const string
nit: could be a compile-time constant, i.e.

constexpr const char* const kTableName


http://gerrit.cloudera.org:8080/#/c/18794/2/src/kudu/tools/kudu-admin-test.cc@2113
PS2, Line 2113: _p1
nit for here and below: since those are scope-specific variables, you can drop the suffix here and in the scope below for easier readability


http://gerrit.cloudera.org:8080/#/c/18794/2/src/kudu/tools/kudu-admin-test.cc@2126
PS2, Line 2126:         
nit for here and below: wrong indent


http://gerrit.cloudera.org:8080/#/c/18794/1/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:

http://gerrit.cloudera.org:8080/#/c/18794/1/src/kudu/tools/tool_action_table.cc@291
PS1, Line 291: const char* const kReplicationFactorArg = "replication_factor";
> No, it shouldn't be there. Thanks for catching it.
I guess we should thank the TidyBot for catching this :)



-- 
To view, visit http://gerrit.cloudera.org:8080/18794
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9fa2eb965051a5f63d1c482e6fd43ff654ec6364
Gerrit-Change-Number: 18794
Gerrit-PatchSet: 2
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 28 Jul 2022 00:01:36 +0000
Gerrit-HasComments: Yes

[kudu-CR] [tools] KUDU-2671 Update the kudu table describe tool

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18794 )

Change subject: [tools] KUDU-2671 Update the kudu table describe tool
......................................................................


Patch Set 1: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/18794/1/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:

http://gerrit.cloudera.org:8080/#/c/18794/1/src/kudu/tools/tool_action_table.cc@291
PS1, Line 291: const char* const kHashSchemaJSONArg = "create_table_json";
> warning: unused variable 'kHashSchemaJSONArg' [clang-diagnostic-unused-cons
Is this needed?



-- 
To view, visit http://gerrit.cloudera.org:8080/18794
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9fa2eb965051a5f63d1c482e6fd43ff654ec6364
Gerrit-Change-Number: 18794
Gerrit-PatchSet: 1
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 27 Jul 2022 22:41:31 +0000
Gerrit-HasComments: Yes

[kudu-CR] [tools] KUDU-2671 Update the kudu table describe tool

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18794 )

Change subject: [tools] KUDU-2671 Update the kudu table describe tool
......................................................................


Patch Set 3:

> As discussed we want to have the output of the kudu table describe
 > close to the current schema/partition schema of the table by
 > default from a user perspective.

Yes, that makes sense to me.


-- 
To view, visit http://gerrit.cloudera.org:8080/18794
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9fa2eb965051a5f63d1c482e6fd43ff654ec6364
Gerrit-Change-Number: 18794
Gerrit-PatchSet: 3
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 28 Jul 2022 21:09:23 +0000
Gerrit-HasComments: No

[kudu-CR] [tools] KUDU-2671 Update the kudu table describe tool

Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Hello Mahesh Reddy, Tidy Bot, Alexey Serbin, Kudu Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/18794

to look at the new patch set (#3).

Change subject: [tools] KUDU-2671 Update the kudu table describe tool
......................................................................

[tools] KUDU-2671 Update the kudu table describe tool

This patch updates the kudu table describe tool to output ranges
which have a custom hash schema with the corresponding hash schema.
If a partition has multiple custom hash dimensions they are
separated by a space.

Change-Id: I9fa2eb965051a5f63d1c482e6fd43ff654ec6364
---
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/tool_action_table.cc
2 files changed, 61 insertions(+), 3 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/18794/3
-- 
To view, visit http://gerrit.cloudera.org:8080/18794
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9fa2eb965051a5f63d1c482e6fd43ff654ec6364
Gerrit-Change-Number: 18794
Gerrit-PatchSet: 3
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] [tools] KUDU-2671 Update the kudu table describe tool

Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/18794 )

Change subject: [tools] KUDU-2671 Update the kudu table describe tool
......................................................................


Patch Set 3:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/18794/2/src/kudu/tools/kudu-admin-test.cc
File src/kudu/tools/kudu-admin-test.cc:

http://gerrit.cloudera.org:8080/#/c/18794/2/src/kudu/tools/kudu-admin-test.cc@2103
PS2, Line 2103: constexpr co
> nit: could be a compile-time constant, i.e.
Done


http://gerrit.cloudera.org:8080/#/c/18794/2/src/kudu/tools/kudu-admin-test.cc@2113
PS2, Line 2113: (sc
> nit for here and below: since those are scope-specific variables, you can d
Done


http://gerrit.cloudera.org:8080/#/c/18794/2/src/kudu/tools/kudu-admin-test.cc@2126
PS2, Line 2126:     CHEC
> nit for here and below: wrong indent
Done



-- 
To view, visit http://gerrit.cloudera.org:8080/18794
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9fa2eb965051a5f63d1c482e6fd43ff654ec6364
Gerrit-Change-Number: 18794
Gerrit-PatchSet: 3
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 28 Jul 2022 15:39:23 +0000
Gerrit-HasComments: Yes

[kudu-CR] [tools] KUDU-2671 Update the kudu table describe tool

Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Hello Mahesh Reddy, Tidy Bot, Alexey Serbin, Kudu Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/18794

to look at the new patch set (#2).

Change subject: [tools] KUDU-2671 Update the kudu table describe tool
......................................................................

[tools] KUDU-2671 Update the kudu table describe tool

This patch updates the kudu table describe tool to output ranges
which have a custom hash schema with the corresponding hash schema.
If a partition has multiple custom hash dimensions they are
separated by a space.

Change-Id: I9fa2eb965051a5f63d1c482e6fd43ff654ec6364
---
M cmake_modules/KuduLinker.cmake
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/tool_action_table.cc
3 files changed, 62 insertions(+), 4 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/18794/2
-- 
To view, visit http://gerrit.cloudera.org:8080/18794
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9fa2eb965051a5f63d1c482e6fd43ff654ec6364
Gerrit-Change-Number: 18794
Gerrit-PatchSet: 2
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] [tools] KUDU-2671 Update the kudu table describe tool

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18794 )

Change subject: [tools] KUDU-2671 Update the kudu table describe tool
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/18794/1/src/kudu/tools/kudu-admin-test.cc
File src/kudu/tools/kudu-admin-test.cc:

http://gerrit.cloudera.org:8080/#/c/18794/1/src/kudu/tools/kudu-admin-test.cc@2114
PS1, Line 2114:         
here and below: wrong indent



-- 
To view, visit http://gerrit.cloudera.org:8080/18794
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9fa2eb965051a5f63d1c482e6fd43ff654ec6364
Gerrit-Change-Number: 18794
Gerrit-PatchSet: 1
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 27 Jul 2022 22:42:23 +0000
Gerrit-HasComments: Yes

[kudu-CR] [tools] KUDU-2671 Update the kudu table describe tool

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/18794 )

Change subject: [tools] KUDU-2671 Update the kudu table describe tool
......................................................................

[tools] KUDU-2671 Update the kudu table describe tool

This patch updates the kudu table describe tool to output ranges
which have a custom hash schema with the corresponding hash schema.
If a partition has multiple custom hash dimensions they are
separated by a space.

Change-Id: I9fa2eb965051a5f63d1c482e6fd43ff654ec6364
Reviewed-on: http://gerrit.cloudera.org:8080/18794
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <al...@apache.org>
---
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/tool_action_table.cc
2 files changed, 61 insertions(+), 3 deletions(-)

Approvals:
  Kudu Jenkins: Verified
  Alexey Serbin: Looks good to me, approved

-- 
To view, visit http://gerrit.cloudera.org:8080/18794
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I9fa2eb965051a5f63d1c482e6fd43ff654ec6364
Gerrit-Change-Number: 18794
Gerrit-PatchSet: 4
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)