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 <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Thu, 11 Oct 2018 17:30:00 +0000 Gerrit-HasComments: Yes
