You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Yuqi Du (Code Review)" <ge...@cloudera.org> on 2023/03/13 03:27:54 UTC

[kudu-CR] [ut] make a checking command of 'TestTServerListState' more meaningful

Yuqi Du has uploaded this change for review. ( http://gerrit.cloudera.org:8080/19611


Change subject: [ut] make a checking command of 'TestTServerListState' more meaningful
......................................................................

[ut] make a checking command of 'TestTServerListState' more meaningful

This patch is not important. An useless checking command found
when I review codes, because the checking command is always true
So, improve it.

Change-Id: I2cc3c16cba812eaaa8786cea37659fd5e9267fd3
---
M src/kudu/tools/kudu-tool-test.cc
1 file changed, 1 insertion(+), 1 deletion(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I2cc3c16cba812eaaa8786cea37659fd5e9267fd3
Gerrit-Change-Number: 19611
Gerrit-PatchSet: 1
Gerrit-Owner: Yuqi Du <sh...@gmail.com>

[kudu-CR] [ut] make a checking command of 'TestTServerListState' more meaningful

Posted by "Yuqi Du (Code Review)" <ge...@cloudera.org>.
Yuqi Du has restored this change. ( http://gerrit.cloudera.org:8080/19611 )

Change subject: [ut] make a checking command of 'TestTServerListState' more meaningful
......................................................................


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: restore
Gerrit-Change-Id: I2cc3c16cba812eaaa8786cea37659fd5e9267fd3
Gerrit-Change-Number: 19611
Gerrit-PatchSet: 1
Gerrit-Owner: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] [ut] make a checking command of 'TestTServerListState' more meaningful

Posted by "Yuqi Du (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, 

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

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

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

Change subject: [ut] make a checking command of 'TestTServerListState' more meaningful
......................................................................

[ut] make a checking command of 'TestTServerListState' more meaningful

This patch is not important. An useless checking command found
when I review codes, because the checking command is always true
So, improve it.

Change-Id: I2cc3c16cba812eaaa8786cea37659fd5e9267fd3
---
M src/kudu/tools/kudu-tool-test.cc
1 file changed, 6 insertions(+), 3 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2cc3c16cba812eaaa8786cea37659fd5e9267fd3
Gerrit-Change-Number: 19611
Gerrit-PatchSet: 3
Gerrit-Owner: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] [ut] make a checking command of 'TestTServerListState' more meaningful

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

Change subject: [ut] make a checking command of 'TestTServerListState' more meaningful
......................................................................


Patch Set 5: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2cc3c16cba812eaaa8786cea37659fd5e9267fd3
Gerrit-Change-Number: 19611
Gerrit-PatchSet: 5
Gerrit-Owner: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Wed, 22 Mar 2023 14:53:52 +0000
Gerrit-HasComments: No

[kudu-CR] [ut] make a checking command of 'TestTServerListState' more meaningful

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

Change subject: [ut] make a checking command of 'TestTServerListState' more meaningful
......................................................................


Patch Set 4: Code-Review+1

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/19611/4/src/kudu/tools/kudu-tool-test.cc@4890
PS4, Line 4890: If the state isn't requested, we shouldn't see any.
nit: comments need update.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2cc3c16cba812eaaa8786cea37659fd5e9267fd3
Gerrit-Change-Number: 19611
Gerrit-PatchSet: 4
Gerrit-Owner: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Tue, 14 Mar 2023 09:40:00 +0000
Gerrit-HasComments: Yes

[kudu-CR] [ut] make a checking command of 'TestTServerListState' more meaningful

Posted by "Yuqi Du (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, 

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

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

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

Change subject: [ut] make a checking command of 'TestTServerListState' more meaningful
......................................................................

[ut] make a checking command of 'TestTServerListState' more meaningful

This patch is not important. An useless checking command found
when I review codes, because the checking command is always true
So, improve it.

Change-Id: I2cc3c16cba812eaaa8786cea37659fd5e9267fd3
---
M src/kudu/tools/kudu-tool-test.cc
1 file changed, 5 insertions(+), 2 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2cc3c16cba812eaaa8786cea37659fd5e9267fd3
Gerrit-Change-Number: 19611
Gerrit-PatchSet: 2
Gerrit-Owner: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] [ut] make a checking command of 'TestTServerListState' more meaningful

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

Change subject: [ut] make a checking command of 'TestTServerListState' more meaningful
......................................................................


Patch Set 4:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/19611/4/src/kudu/tools/kudu-tool-test.cc@4891
PS4, Line 4891: ASSERT_STR_NOT_CONTAINS(out, Substitute("$0,$1", ts_uuid_noop, "MAINTENANCE_MODE"));
If we set the ts number for 3 in line 4876, how about add a loop check in here, and exclude the node that executed the 'enter_maintenance' command which is cluster_->tablet_server(0).



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2cc3c16cba812eaaa8786cea37659fd5e9267fd3
Gerrit-Change-Number: 19611
Gerrit-PatchSet: 4
Gerrit-Owner: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Tue, 14 Mar 2023 12:00:02 +0000
Gerrit-HasComments: Yes

[kudu-CR] [ut] make a checking command of 'TestTServerListState' more meaningful

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

Change subject: [ut] make a checking command of 'TestTServerListState' more meaningful
......................................................................

[ut] make a checking command of 'TestTServerListState' more meaningful

This patch is not important. An useless checking command found
when I review codes, because the checking command is always true
So, improve it.

Change-Id: I2cc3c16cba812eaaa8786cea37659fd5e9267fd3
Reviewed-on: http://gerrit.cloudera.org:8080/19611
Tested-by: Kudu Jenkins
Reviewed-by: Yingchun Lai <la...@apache.org>
Reviewed-by: Alexey Serbin <al...@apache.org>
---
M src/kudu/tools/kudu-tool-test.cc
1 file changed, 11 insertions(+), 6 deletions(-)

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

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I2cc3c16cba812eaaa8786cea37659fd5e9267fd3
Gerrit-Change-Number: 19611
Gerrit-PatchSet: 6
Gerrit-Owner: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>

[kudu-CR] [ut] make a checking command of 'TestTServerListState' more meaningful

Posted by "Yuqi Du (Code Review)" <ge...@cloudera.org>.
Yuqi Du has abandoned this change. ( http://gerrit.cloudera.org:8080/19611 )

Change subject: [ut] make a checking command of 'TestTServerListState' more meaningful
......................................................................


Abandoned

abandon it
-- 
To view, visit http://gerrit.cloudera.org:8080/19611
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: I2cc3c16cba812eaaa8786cea37659fd5e9267fd3
Gerrit-Change-Number: 19611
Gerrit-PatchSet: 1
Gerrit-Owner: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] [ut] make a checking command of 'TestTServerListState' more meaningful

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

Change subject: [ut] make a checking command of 'TestTServerListState' more meaningful
......................................................................


Patch Set 5: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2cc3c16cba812eaaa8786cea37659fd5e9267fd3
Gerrit-Change-Number: 19611
Gerrit-PatchSet: 5
Gerrit-Owner: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Wed, 22 Mar 2023 15:33:55 +0000
Gerrit-HasComments: No

[kudu-CR] [ut] make a checking command of 'TestTServerListState' more meaningful

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

Change subject: [ut] make a checking command of 'TestTServerListState' more meaningful
......................................................................


Patch Set 3:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/19611/3/src/kudu/tools/kudu-tool-test.cc@4889
PS3, Line 4889: ,state
I guess the previous purpose is check the 'state' column should not apear if not request, of course it can be covered by ToolTest.TestTserverList.
To improve the test, would it better to run the command once, then use ASSERT_STR_NOT_CONTAINS to check ts_uuid_noop and use ASSERT_STR_CONTAINS to check ts_uuid together?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2cc3c16cba812eaaa8786cea37659fd5e9267fd3
Gerrit-Change-Number: 19611
Gerrit-PatchSet: 3
Gerrit-Owner: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
Gerrit-Comment-Date: Tue, 14 Mar 2023 03:52:13 +0000
Gerrit-HasComments: Yes

[kudu-CR] [ut] make a checking command of 'TestTServerListState' more meaningful

Posted by "Yuqi Du (Code Review)" <ge...@cloudera.org>.
Hello Yingchun Lai, Kudu Jenkins, 

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

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

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

Change subject: [ut] make a checking command of 'TestTServerListState' more meaningful
......................................................................

[ut] make a checking command of 'TestTServerListState' more meaningful

This patch is not important. An useless checking command found
when I review codes, because the checking command is always true
So, improve it.

Change-Id: I2cc3c16cba812eaaa8786cea37659fd5e9267fd3
---
M src/kudu/tools/kudu-tool-test.cc
1 file changed, 8 insertions(+), 6 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/11/19611/4
-- 
To view, visit http://gerrit.cloudera.org:8080/19611
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2cc3c16cba812eaaa8786cea37659fd5e9267fd3
Gerrit-Change-Number: 19611
Gerrit-PatchSet: 4
Gerrit-Owner: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>

[kudu-CR] [ut] make a checking command of 'TestTServerListState' more meaningful

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

Change subject: [ut] make a checking command of 'TestTServerListState' more meaningful
......................................................................


Patch Set 4:

(1 comment)

> Patch Set 3:
> 
> (1 comment)

done

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

http://gerrit.cloudera.org:8080/#/c/19611/3/src/kudu/tools/kudu-tool-test.cc@4889
PS3, Line 4889: 
> I guess the previous purpose is check the 'state' column should not apear i
good!



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2cc3c16cba812eaaa8786cea37659fd5e9267fd3
Gerrit-Change-Number: 19611
Gerrit-PatchSet: 4
Gerrit-Owner: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Tue, 14 Mar 2023 08:25:51 +0000
Gerrit-HasComments: Yes

[kudu-CR] [ut] make a checking command of 'TestTServerListState' more meaningful

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

Change subject: [ut] make a checking command of 'TestTServerListState' more meaningful
......................................................................


Patch Set 5:

(2 comments)

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

http://gerrit.cloudera.org:8080/#/c/19611/4/src/kudu/tools/kudu-tool-test.cc@4890
PS4, Line 4890:  (int i = 1; i < kTServerNum; i++) {
> nit: comments need update.
Done


http://gerrit.cloudera.org:8080/#/c/19611/4/src/kudu/tools/kudu-tool-test.cc@4891
PS4, Line 4891:   // If a ts isn't requested, we shouldn't see its maintenance state.
> If we set the ts number for 3 in line 4876, how about add a loop check in h
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2cc3c16cba812eaaa8786cea37659fd5e9267fd3
Gerrit-Change-Number: 19611
Gerrit-PatchSet: 5
Gerrit-Owner: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Wed, 22 Mar 2023 09:37:05 +0000
Gerrit-HasComments: Yes

[kudu-CR] [ut] make a checking command of 'TestTServerListState' more meaningful

Posted by "Yuqi Du (Code Review)" <ge...@cloudera.org>.
Hello Yingchun Lai, Kudu Jenkins, KeDeng, 

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

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

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

Change subject: [ut] make a checking command of 'TestTServerListState' more meaningful
......................................................................

[ut] make a checking command of 'TestTServerListState' more meaningful

This patch is not important. An useless checking command found
when I review codes, because the checking command is always true
So, improve it.

Change-Id: I2cc3c16cba812eaaa8786cea37659fd5e9267fd3
---
M src/kudu/tools/kudu-tool-test.cc
1 file changed, 11 insertions(+), 6 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/11/19611/5
-- 
To view, visit http://gerrit.cloudera.org:8080/19611
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2cc3c16cba812eaaa8786cea37659fd5e9267fd3
Gerrit-Change-Number: 19611
Gerrit-PatchSet: 5
Gerrit-Owner: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>