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/10 03:56:33 UTC
[kudu-CR] [test] use LocationInfo in ts itest base
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11643
Change subject: [test] use LocationInfo in ts_itest_base
......................................................................
[test] use LocationInfo in ts_itest_base
Added LocationInfo parameter into TabletServerIntegrationTestBase
methods: BuildAndStart() and CreateCluster().
In addition, I did some minor refactoring to use move semantics
for the parameters of the methods mentioned above.
Change-Id: If6fb1f583e43b9e64e3c396b7be1977546e71347
---
M src/kudu/integration-tests/linked_list-test.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/integration-tests/raft_consensus_election-itest.cc
M src/kudu/integration-tests/ts_itest-base.cc
M src/kudu/integration-tests/ts_itest-base.h
5 files changed, 35 insertions(+), 28 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/11643/1
--
To view, visit http://gerrit.cloudera.org:8080/11643
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: If6fb1f583e43b9e64e3c396b7be1977546e71347
Gerrit-Change-Number: 11643
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [test] use LocationInfo in ts itest base
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11643 )
Change subject: [test] use LocationInfo in ts_itest_base
......................................................................
Patch Set 1:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/11643/1/src/kudu/integration-tests/ts_itest-base.h
File src/kudu/integration-tests/ts_itest-base.h:
http://gerrit.cloudera.org:8080/#/c/11643/1/src/kudu/integration-tests/ts_itest-base.h@56
PS1, Line 56: std::vector<std::string> non_default_ts_flags
> Why not a default value of {} for this? I see that in practice we always sp
Yep, I left it non-default based on the observation that most times some non-default flags are specified.
You are right -- from the pure interface perspective having default {} here makes the signature of this method more consistent.
Done.
http://gerrit.cloudera.org:8080/#/c/11643/1/src/kudu/integration-tests/ts_itest-base.cc
File src/kudu/integration-tests/ts_itest-base.cc:
http://gerrit.cloudera.org:8080/#/c/11643/1/src/kudu/integration-tests/ts_itest-base.cc@a111
PS1, Line 111:
> I guess it was never used?
Yep, num_data_dirs is not used in the current code. I think that would be easy adding it back if it's needed.
http://gerrit.cloudera.org:8080/#/c/11643/1/src/kudu/integration-tests/ts_itest-base.cc@134
PS1, Line 134: auto&&
> Extra &?
woops, done
http://gerrit.cloudera.org:8080/#/c/11643/1/src/kudu/integration-tests/ts_itest-base.cc@138
PS1, Line 138: auto&&
> Here too.
Done
--
To view, visit http://gerrit.cloudera.org:8080/11643
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If6fb1f583e43b9e64e3c396b7be1977546e71347
Gerrit-Change-Number: 11643
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Thu, 11 Oct 2018 17:30:00 +0000
Gerrit-HasComments: Yes
[kudu-CR] [test] use LocationInfo in ts itest base
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11643 )
Change subject: [test] use LocationInfo in ts_itest_base
......................................................................
[test] use LocationInfo in ts_itest_base
Added LocationInfo parameter into TabletServerIntegrationTestBase
methods: BuildAndStart() and CreateCluster().
In addition, I did some minor refactoring to use move semantics
for the parameters of the methods mentioned above.
Change-Id: If6fb1f583e43b9e64e3c396b7be1977546e71347
Reviewed-on: http://gerrit.cloudera.org:8080/11643
Reviewed-by: Will Berkeley <wd...@gmail.com>
Tested-by: Alexey Serbin <as...@cloudera.com>
---
M src/kudu/integration-tests/linked_list-test.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/integration-tests/raft_consensus_election-itest.cc
M src/kudu/integration-tests/ts_itest-base.cc
M src/kudu/integration-tests/ts_itest-base.h
5 files changed, 35 insertions(+), 28 deletions(-)
Approvals:
Will Berkeley: Looks good to me, approved
Alexey Serbin: Verified
--
To view, visit http://gerrit.cloudera.org:8080/11643
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: If6fb1f583e43b9e64e3c396b7be1977546e71347
Gerrit-Change-Number: 11643
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] [test] use LocationInfo in ts itest base
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11643 )
Change subject: [test] use LocationInfo in ts_itest_base
......................................................................
Patch Set 2: Verified+1
Unrelated flake in org.apache.kudu.spark.kudu.DefaultSourceTest
--
To view, visit http://gerrit.cloudera.org:8080/11643
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If6fb1f583e43b9e64e3c396b7be1977546e71347
Gerrit-Change-Number: 11643
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Thu, 11 Oct 2018 18:15:09 +0000
Gerrit-HasComments: No
[kudu-CR] [test] use LocationInfo in ts itest base
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11643
to look at the new patch set (#2).
Change subject: [test] use LocationInfo in ts_itest_base
......................................................................
[test] use LocationInfo in ts_itest_base
Added LocationInfo parameter into TabletServerIntegrationTestBase
methods: BuildAndStart() and CreateCluster().
In addition, I did some minor refactoring to use move semantics
for the parameters of the methods mentioned above.
Change-Id: If6fb1f583e43b9e64e3c396b7be1977546e71347
---
M src/kudu/integration-tests/linked_list-test.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/integration-tests/raft_consensus_election-itest.cc
M src/kudu/integration-tests/ts_itest-base.cc
M src/kudu/integration-tests/ts_itest-base.h
5 files changed, 35 insertions(+), 28 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/11643/2
--
To view, visit http://gerrit.cloudera.org:8080/11643
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If6fb1f583e43b9e64e3c396b7be1977546e71347
Gerrit-Change-Number: 11643
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] [test] use LocationInfo in ts itest base
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/11643 )
Change subject: [test] use LocationInfo in ts_itest_base
......................................................................
Removed reviewer Kudu Jenkins with the following votes:
* Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/11643
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: If6fb1f583e43b9e64e3c396b7be1977546e71347
Gerrit-Change-Number: 11643
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] [test] use LocationInfo in ts itest base
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11643 )
Change subject: [test] use LocationInfo in ts_itest_base
......................................................................
Patch Set 1:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/11643/1/src/kudu/integration-tests/ts_itest-base.h
File src/kudu/integration-tests/ts_itest-base.h:
http://gerrit.cloudera.org:8080/#/c/11643/1/src/kudu/integration-tests/ts_itest-base.h@56
PS1, Line 56: std::vector<std::string> non_default_ts_flags
Why not a default value of {} for this? I see that in practice we always specify `non_default_ts_flags`, but logically it's consistent to have the {} default if `non_default_master_flags` does.
http://gerrit.cloudera.org:8080/#/c/11643/1/src/kudu/integration-tests/ts_itest-base.cc
File src/kudu/integration-tests/ts_itest-base.cc:
http://gerrit.cloudera.org:8080/#/c/11643/1/src/kudu/integration-tests/ts_itest-base.cc@a111
PS1, Line 111:
I guess it was never used?
http://gerrit.cloudera.org:8080/#/c/11643/1/src/kudu/integration-tests/ts_itest-base.cc@134
PS1, Line 134: auto&&
Extra &?
http://gerrit.cloudera.org:8080/#/c/11643/1/src/kudu/integration-tests/ts_itest-base.cc@138
PS1, Line 138: auto&&
Here too.
--
To view, visit http://gerrit.cloudera.org:8080/11643
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If6fb1f583e43b9e64e3c396b7be1977546e71347
Gerrit-Change-Number: 11643
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Thu, 11 Oct 2018 16:48:36 +0000
Gerrit-HasComments: Yes
[kudu-CR] [test] use LocationInfo in ts itest base
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11643 )
Change subject: [test] use LocationInfo in ts_itest_base
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/11643
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If6fb1f583e43b9e64e3c396b7be1977546e71347
Gerrit-Change-Number: 11643
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Thu, 11 Oct 2018 17:36:57 +0000
Gerrit-HasComments: No