You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "helifu (Code Review)" <ge...@cloudera.org> on 2019/10/29 13:18:29 UTC

[kudu-CR] KUDU-2986 p1: adjust the output of "table statistics" from 0 -> N/A 

helifu has uploaded this change for review. ( http://gerrit.cloudera.org:8080/14567


Change subject: KUDU-2986 p1: adjust the output of "table statistics" from 0 -> N/A
......................................................................

KUDU-2986 p1: adjust the output of "table statistics" from 0 -> N/A

Change-Id: I82c47f5ac07cf63d455c0720614c5383bfa88ca3
---
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/table_statistics-internal.h
3 files changed, 14 insertions(+), 6 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I82c47f5ac07cf63d455c0720614c5383bfa88ca3
Gerrit-Change-Number: 14567
Gerrit-PatchSet: 1
Gerrit-Owner: helifu <hz...@corp.netease.com>

[kudu-CR] KUDU-2986 p1: adjust the output of "table statistics" from 0 -> N/A

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

Change subject: KUDU-2986 p1: adjust the output of "table statistics" from 0 -> N/A
......................................................................


Patch Set 1:

(1 comment)

Do we need to update the java client?

https://github.com/apache/kudu/blob/6ecfe60518803d3963674bf6172f242b5de54780/src/kudu/master/catalog_manager.cc#L2942
https://github.com/apache/kudu/blob/6ecfe60518803d3963674bf6172f242b5de54780/java/kudu-client/src/main/java/org/apache/kudu/client/GetTableStatisticsRequest.java#L72

http://gerrit.cloudera.org:8080/#/c/14567/1/src/kudu/client/table_statistics-internal.h
File src/kudu/client/table_statistics-internal.h:

http://gerrit.cloudera.org:8080/#/c/14567/1/src/kudu/client/table_statistics-internal.h@47
PS1, Line 47:     if (live_row_count_) {
            :       display_string += Substitute("live row count: $0\n", *live_row_count_);
            :     } else {
            :       display_string += Substitute("live row count: N/A\n");
            :     }
> Rewrite as a ternary:
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I82c47f5ac07cf63d455c0720614c5383bfa88ca3
Gerrit-Change-Number: 14567
Gerrit-PatchSet: 1
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Wed, 30 Oct 2019 02:20:20 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2986 p1: adjust the output of "table statistics" from 0 -> N/A

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

Change subject: KUDU-2986 p1: adjust the output of "table statistics" from 0 -> N/A
......................................................................


Patch Set 2:

(9 comments)

http://gerrit.cloudera.org:8080/#/c/14567/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/14567/2//COMMIT_MSG@8
PS2, Line 8: 
Write a meaningful description: what is contained in 'KUDU-2986 p1'.


http://gerrit.cloudera.org:8080/#/c/14567/2/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:

http://gerrit.cloudera.org:8080/#/c/14567/2/src/kudu/client/client-test.cc@805
PS2, Line 805: Support
What supports what?  Please write a meaningful sentence here.


http://gerrit.cloudera.org:8080/#/c/14567/2/src/kudu/client/client-test.cc@809
PS2, Line 809: Not support live row count.
What doesn't supports what?  Please write a meaningful sentence here.


http://gerrit.cloudera.org:8080/#/c/14567/2/src/kudu/client/client-test.cc@813
PS2, Line 813:   ASSERT_EQ(-1, statistics->live_row_count());
How do we know FLAGS_live_row_count_for_testing is not -1?  Please add

ASSERT_NE(-1, FLAGS_live_row_count_for_testing);


http://gerrit.cloudera.org:8080/#/c/14567/1/src/kudu/client/client.h
File src/kudu/client/client.h:

http://gerrit.cloudera.org:8080/#/c/14567/1/src/kudu/client/client.h@974
PS1, Line 974:   ///  -1 is returned if the table doesn't support live_row_count.
Move this under the @return section.


http://gerrit.cloudera.org:8080/#/c/14567/1/src/kudu/client/table_statistics-internal.h
File src/kudu/client/table_statistics-internal.h:

http://gerrit.cloudera.org:8080/#/c/14567/1/src/kudu/client/table_statistics-internal.h@45
PS1, Line 45:  = ""
While you are at it, please drop this assignment since it's redundant.  The default string constructor produces an empty string.


http://gerrit.cloudera.org:8080/#/c/14567/2/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:

http://gerrit.cloudera.org:8080/#/c/14567/2/src/kudu/master/catalog_manager.cc@278
PS2, Line 278: support_live_row_count_for_testing
Please follow the convention for naming flags in this file.  This is catalog_manager, so name it like catalog_manager_support_live_row_count.  'for_testing' is redundant -- it should be in description and the flag is hidden.


http://gerrit.cloudera.org:8080/#/c/14567/2/src/kudu/master/catalog_manager.cc@279
PS2, Line 279: Whether to enable mock support live row count for testing.
Whether to enable mock live row count statistic for tables. For testing only.


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

http://gerrit.cloudera.org:8080/#/c/14567/2/src/kudu/tools/kudu-tool-test.cc@5574
PS2, Line 5574: TEST_F(ToolTest, TestGetTableStatisticsLiveRowCountNotSupported) {
Is there a test to verify functionality of the live row count stats collection when it's enabled?  If not, please add one.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I82c47f5ac07cf63d455c0720614c5383bfa88ca3
Gerrit-Change-Number: 14567
Gerrit-PatchSet: 2
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Wed, 30 Oct 2019 02:50:40 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2986 p1: adjust the output of "table statistics" from 0 -> N/A

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

Change subject: KUDU-2986 p1: adjust the output of "table statistics" from 0 -> N/A
......................................................................


Patch Set 2:

(9 comments)

http://gerrit.cloudera.org:8080/#/c/14567/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/14567/2//COMMIT_MSG@8
PS2, Line 8: 
> Write a meaningful description: what is contained in 'KUDU-2986 p1'.
Done


http://gerrit.cloudera.org:8080/#/c/14567/2/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:

http://gerrit.cloudera.org:8080/#/c/14567/2/src/kudu/client/client-test.cc@805
PS2, Line 805: Support
> What supports what?  Please write a meaningful sentence here.
Done


http://gerrit.cloudera.org:8080/#/c/14567/2/src/kudu/client/client-test.cc@809
PS2, Line 809: Not support live row count.
> What doesn't supports what?  Please write a meaningful sentence here.
Done


http://gerrit.cloudera.org:8080/#/c/14567/2/src/kudu/client/client-test.cc@813
PS2, Line 813:   ASSERT_EQ(-1, statistics->live_row_count());
> How do we know FLAGS_live_row_count_for_testing is not -1?  Please add
Done


http://gerrit.cloudera.org:8080/#/c/14567/1/src/kudu/client/client.h
File src/kudu/client/client.h:

http://gerrit.cloudera.org:8080/#/c/14567/1/src/kudu/client/client.h@974
PS1, Line 974:   ///  -1 is returned if the table doesn't support live_row_count.
> Move this under the @return section.
Done


http://gerrit.cloudera.org:8080/#/c/14567/1/src/kudu/client/table_statistics-internal.h
File src/kudu/client/table_statistics-internal.h:

http://gerrit.cloudera.org:8080/#/c/14567/1/src/kudu/client/table_statistics-internal.h@45
PS1, Line 45:  = ""
> While you are at it, please drop this assignment since it's redundant.  The
Done


http://gerrit.cloudera.org:8080/#/c/14567/2/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:

http://gerrit.cloudera.org:8080/#/c/14567/2/src/kudu/master/catalog_manager.cc@278
PS2, Line 278: support_live_row_count_for_testing
> Please follow the convention for naming flags in this file.  This is catalo
Done


http://gerrit.cloudera.org:8080/#/c/14567/2/src/kudu/master/catalog_manager.cc@279
PS2, Line 279: Whether to enable mock support live row count for testing.
> Whether to enable mock live row count statistic for tables. For testing onl
Done


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

http://gerrit.cloudera.org:8080/#/c/14567/2/src/kudu/tools/kudu-tool-test.cc@5574
PS2, Line 5574: TEST_F(ToolTest, TestGetTableStatisticsLiveRowCountNotSupported) {
> Is there a test to verify functionality of the live row count stats collect
Yep, I think below two test cases are enough:
https://github.com/apache/kudu/blob/0870259456f785c4efdf5826350166d8d6dff5cc/src/kudu/integration-tests/ts_tablet_manager-itest.cc#L699
https://github.com/apache/kudu/blob/0870259456f785c4efdf5826350166d8d6dff5cc/src/kudu/master/master-test.cc#L1778



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I82c47f5ac07cf63d455c0720614c5383bfa88ca3
Gerrit-Change-Number: 14567
Gerrit-PatchSet: 2
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Wed, 30 Oct 2019 07:33:29 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2986 p1: adjust the kudu CLI output from 0 to N/A

Posted by "helifu (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, 

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

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

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

Change subject: KUDU-2986 p1: adjust the kudu CLI output from 0 to N/A
......................................................................

KUDU-2986 p1: adjust the kudu CLI output from 0 to N/A

The `live_row_count` metric value should be invalid(N/A) in
kudu CLI output("kudu table statistics") while there is any
tablet which doesn't support live row count.

Change-Id: I82c47f5ac07cf63d455c0720614c5383bfa88ca3
---
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/table_statistics-internal.h
M src/kudu/master/catalog_manager.cc
M src/kudu/tools/kudu-tool-test.cc
6 files changed, 36 insertions(+), 14 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I82c47f5ac07cf63d455c0720614c5383bfa88ca3
Gerrit-Change-Number: 14567
Gerrit-PatchSet: 3
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>

[kudu-CR] KUDU-2986 p1: adjust the kudu CLI output from 0 to N/A

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

Change subject: KUDU-2986 p1: adjust the kudu CLI output from 0 to N/A
......................................................................

KUDU-2986 p1: adjust the kudu CLI output from 0 to N/A

The `live_row_count` metric value should be invalid(N/A) in
kudu CLI output("kudu table statistics") while there is any
tablet which doesn't support live row count.

Change-Id: I82c47f5ac07cf63d455c0720614c5383bfa88ca3
Reviewed-on: http://gerrit.cloudera.org:8080/14567
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <as...@cloudera.com>
---
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/table_statistics-internal.h
M src/kudu/master/catalog_manager.cc
M src/kudu/tools/kudu-tool-test.cc
6 files changed, 36 insertions(+), 14 deletions(-)

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

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I82c47f5ac07cf63d455c0720614c5383bfa88ca3
Gerrit-Change-Number: 14567
Gerrit-PatchSet: 4
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>

[kudu-CR] KUDU-2986 p1: adjust the output of "table statistics" from 0 -> N/A

Posted by "helifu (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo, 

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

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

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

Change subject: KUDU-2986 p1: adjust the output of "table statistics" from 0 -> N/A
......................................................................

KUDU-2986 p1: adjust the output of "table statistics" from 0 -> N/A

Change-Id: I82c47f5ac07cf63d455c0720614c5383bfa88ca3
---
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/table_statistics-internal.h
M src/kudu/master/catalog_manager.cc
M src/kudu/tools/kudu-tool-test.cc
6 files changed, 33 insertions(+), 11 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I82c47f5ac07cf63d455c0720614c5383bfa88ca3
Gerrit-Change-Number: 14567
Gerrit-PatchSet: 2
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>

[kudu-CR] KUDU-2986 p1: adjust the kudu CLI output from 0 to N/A

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

Change subject: KUDU-2986 p1: adjust the kudu CLI output from 0 to N/A
......................................................................


Patch Set 3: Code-Review+2

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/14567/2/src/kudu/tools/kudu-tool-test.cc@5574
PS2, Line 5574: TEST_F(ToolTest, TestGetTableStatisticsLiveRowCountNotSupported) {
> Yep, I think below two test cases are enough:
Great, thank you!



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I82c47f5ac07cf63d455c0720614c5383bfa88ca3
Gerrit-Change-Number: 14567
Gerrit-PatchSet: 3
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Sat, 02 Nov 2019 21:15:21 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2986 p1: adjust the output of "table statistics" from 0 -> N/A

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

Change subject: KUDU-2986 p1: adjust the output of "table statistics" from 0 -> N/A
......................................................................


Patch Set 1:

(1 comment)

Could you add a unit test for the new behavior?

http://gerrit.cloudera.org:8080/#/c/14567/1/src/kudu/client/table_statistics-internal.h
File src/kudu/client/table_statistics-internal.h:

http://gerrit.cloudera.org:8080/#/c/14567/1/src/kudu/client/table_statistics-internal.h@47
PS1, Line 47:     if (live_row_count_) {
            :       display_string += Substitute("live row count: $0\n", *live_row_count_);
            :     } else {
            :       display_string += Substitute("live row count: N/A\n");
            :     }
Rewrite as a ternary:

  display_string += Substitute("live row count: $0\n", live_row_count_ ? std::to_string(*live_row_count_) : "N/A");



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I82c47f5ac07cf63d455c0720614c5383bfa88ca3
Gerrit-Change-Number: 14567
Gerrit-PatchSet: 1
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 29 Oct 2019 16:30:16 +0000
Gerrit-HasComments: Yes