You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2018/10/30 17:49:54 UTC

[kudu-CR] [test] handle ScanTableToStrings() result status 

Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11825


Change subject: [test] handle ScanTableToStrings() result status
......................................................................

[test] handle ScanTableToStrings() result status

Updated tests to check for the result status of the
kudu::client::ScanTableToStrings() utility function.

Change-Id: I97a68f4c3ba5041848adddbefeb22e64b42a2745
---
M src/kudu/client/client-test-util.cc
M src/kudu/client/client-test-util.h
M src/kudu/client/client-test.cc
M src/kudu/integration-tests/alter_table-test.cc
M src/kudu/integration-tests/flex_partitioning-itest.cc
5 files changed, 48 insertions(+), 45 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I97a68f4c3ba5041848adddbefeb22e64b42a2745
Gerrit-Change-Number: 11825
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>

[kudu-CR] [test] handle ScanTableToStrings() result status

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

Change subject: [test] handle ScanTableToStrings() result status
......................................................................


Patch Set 4: Verified+1

unrelated flake in org.apache.kudu.client.ITClientStress


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I97a68f4c3ba5041848adddbefeb22e64b42a2745
Gerrit-Change-Number: 11825
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Tue, 20 Aug 2019 17:57:49 +0000
Gerrit-HasComments: No

[kudu-CR] [test] handle ScanTableToStrings() result status

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

Change subject: [test] handle ScanTableToStrings() result status
......................................................................

[test] handle ScanTableToStrings() result status

Updated tests to check for the result status of the
kudu::client::ScanTableToStrings() utility function.

Change-Id: I97a68f4c3ba5041848adddbefeb22e64b42a2745
Reviewed-on: http://gerrit.cloudera.org:8080/11825
Tested-by: Alexey Serbin <as...@cloudera.com>
Reviewed-by: Yingchun Lai <40...@qq.com>
---
M src/kudu/client/client-test-util.cc
M src/kudu/client/client-test-util.h
M src/kudu/client/client-test.cc
M src/kudu/integration-tests/alter_table-test.cc
M src/kudu/integration-tests/flex_partitioning-itest.cc
5 files changed, 78 insertions(+), 72 deletions(-)

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

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I97a68f4c3ba5041848adddbefeb22e64b42a2745
Gerrit-Change-Number: 11825
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>

[kudu-CR] [test] handle ScanTableToStrings() result status

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

Change subject: [test] handle ScanTableToStrings() result status
......................................................................


Patch Set 4: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I97a68f4c3ba5041848adddbefeb22e64b42a2745
Gerrit-Change-Number: 11825
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Thu, 22 Aug 2019 02:50:34 +0000
Gerrit-HasComments: No

[kudu-CR] [test] handle ScanTableToStrings() result status

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

Change subject: [test] handle ScanTableToStrings() result status
......................................................................


Patch Set 2:

(2 comments)

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

http://gerrit.cloudera.org:8080/#/c/11825/2/src/kudu/client/client-test.cc@3523
PS2, Line 3523:     ASSERT_OK(ScanTableToStrings(client_table_.get(), &rows));
> Some other place use two ASSERT, compare with rows.size() and rows[0], coul
Done


http://gerrit.cloudera.org:8080/#/c/11825/2/src/kudu/integration-tests/alter_table-test.cc
File src/kudu/integration-tests/alter_table-test.cc:

http://gerrit.cloudera.org:8080/#/c/11825/2/src/kudu/integration-tests/alter_table-test.cc@694
PS2, Line 694:   std::sort(rows->begin(), rows->end());
> Seems many place will sort rows after ScanTableToStrings, how about wrap it
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I97a68f4c3ba5041848adddbefeb22e64b42a2745
Gerrit-Change-Number: 11825
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Fri, 16 Aug 2019 21:13:07 +0000
Gerrit-HasComments: Yes

[kudu-CR] [test] handle ScanTableToStrings() result status

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

Change subject: [test] handle ScanTableToStrings() result status
......................................................................


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/11825/3/src/kudu/client/client-test-util.h
File src/kudu/client/client-test-util.h:

http://gerrit.cloudera.org:8080/#/c/11825/3/src/kudu/client/client-test-util.h@48
PS3, Line 48:   AsIs,
I think enumerators should be named either kEnumName or ENUM_NAME. 
https://google.github.io/styleguide/cppguide.html#Enumerator_Names


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

http://gerrit.cloudera.org:8080/#/c/11825/3/src/kudu/client/client-test.cc@6074
PS3, Line 6074: TEST_F(ClientTest, TestBlockScannerHijackingAttempts) {
These code are not related with commit message, and seem have been merged to master by some other patches, forgot to rebase master branch?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I97a68f4c3ba5041848adddbefeb22e64b42a2745
Gerrit-Change-Number: 11825
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Mon, 19 Aug 2019 12:21:00 +0000
Gerrit-HasComments: Yes

[kudu-CR] [test] handle ScanTableToStrings() result status

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

Change subject: [test] handle ScanTableToStrings() result status
......................................................................


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/11825/3/src/kudu/client/client-test-util.h
File src/kudu/client/client-test-util.h:

http://gerrit.cloudera.org:8080/#/c/11825/3/src/kudu/client/client-test-util.h@48
PS3, Line 48:   AsIs,
> I think enumerators should be named either kEnumName or ENUM_NAME. 
Done


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

http://gerrit.cloudera.org:8080/#/c/11825/3/src/kudu/client/client-test.cc@6074
PS3, Line 6074: TEST_F(ClientTest, TestBlockScannerHijackingAttempts) {
> These code are not related with commit message, and seem have been merged t
I rebased the code on the top of the master branch before posting PS3, and the appearance of this code is the result of the rebase (this particular test scenario was added with changelist e172df405a).  I think that looking at the incremental diff between PS2 and PS3 might be a bit confusing because there were many changes since PS2 was sent for review.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I97a68f4c3ba5041848adddbefeb22e64b42a2745
Gerrit-Change-Number: 11825
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Tue, 20 Aug 2019 17:33:39 +0000
Gerrit-HasComments: Yes

[kudu-CR] [test] handle ScanTableToStrings() result status

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Yingchun Lai, Kudu Jenkins, 

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

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

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

Change subject: [test] handle ScanTableToStrings() result status
......................................................................

[test] handle ScanTableToStrings() result status

Updated tests to check for the result status of the
kudu::client::ScanTableToStrings() utility function.

Change-Id: I97a68f4c3ba5041848adddbefeb22e64b42a2745
---
M src/kudu/client/client-test-util.cc
M src/kudu/client/client-test-util.h
M src/kudu/client/client-test.cc
M src/kudu/integration-tests/alter_table-test.cc
M src/kudu/integration-tests/flex_partitioning-itest.cc
5 files changed, 78 insertions(+), 72 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I97a68f4c3ba5041848adddbefeb22e64b42a2745
Gerrit-Change-Number: 11825
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>

[kudu-CR] [test] handle ScanTableToStrings() result status

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

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

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

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

Change subject: [test] handle ScanTableToStrings() result status
......................................................................

[test] handle ScanTableToStrings() result status

Updated tests to check for the result status of the
kudu::client::ScanTableToStrings() utility function.

Change-Id: I97a68f4c3ba5041848adddbefeb22e64b42a2745
---
M src/kudu/client/client-test-util.cc
M src/kudu/client/client-test-util.h
M src/kudu/client/client-test.cc
M src/kudu/integration-tests/alter_table-test.cc
M src/kudu/integration-tests/flex_partitioning-itest.cc
5 files changed, 49 insertions(+), 48 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I97a68f4c3ba5041848adddbefeb22e64b42a2745
Gerrit-Change-Number: 11825
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] [test] handle ScanTableToStrings() result status

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Yingchun Lai, Kudu Jenkins, 

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

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

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

Change subject: [test] handle ScanTableToStrings() result status
......................................................................

[test] handle ScanTableToStrings() result status

Updated tests to check for the result status of the
kudu::client::ScanTableToStrings() utility function.

Change-Id: I97a68f4c3ba5041848adddbefeb22e64b42a2745
---
M src/kudu/client/client-test-util.cc
M src/kudu/client/client-test-util.h
M src/kudu/client/client-test.cc
M src/kudu/integration-tests/alter_table-test.cc
M src/kudu/integration-tests/flex_partitioning-itest.cc
5 files changed, 78 insertions(+), 72 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I97a68f4c3ba5041848adddbefeb22e64b42a2745
Gerrit-Change-Number: 11825
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>

[kudu-CR] [test] handle ScanTableToStrings() result status

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed a vote on this change.

Change subject: [test] handle ScanTableToStrings() result status
......................................................................


Removed Verified-1 by Kudu Jenkins (120)
-- 
To view, visit http://gerrit.cloudera.org:8080/11825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I97a68f4c3ba5041848adddbefeb22e64b42a2745
Gerrit-Change-Number: 11825
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>

[kudu-CR] [test] handle ScanTableToStrings() result status

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

Change subject: [test] handle ScanTableToStrings() result status
......................................................................


Patch Set 2:

(2 comments)

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

http://gerrit.cloudera.org:8080/#/c/11825/2/src/kudu/client/client-test.cc@3523
PS2, Line 3523:     ASSERT_OK(ScanTableToStrings(client_table_.get(), &rows));
Some other place use two ASSERT, compare with rows.size() and rows[0], could you unify them in the same style?


http://gerrit.cloudera.org:8080/#/c/11825/2/src/kudu/integration-tests/alter_table-test.cc
File src/kudu/integration-tests/alter_table-test.cc:

http://gerrit.cloudera.org:8080/#/c/11825/2/src/kudu/integration-tests/alter_table-test.cc@694
PS2, Line 694:   std::sort(rows->begin(), rows->end());
Seems many place will sort rows after ScanTableToStrings, how about wrap it into the function?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I97a68f4c3ba5041848adddbefeb22e64b42a2745
Gerrit-Change-Number: 11825
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Fri, 26 Jul 2019 00:15:57 +0000
Gerrit-HasComments: Yes