Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/8642 )
Change subject: KUDU-1097 (patch1): test for replica health reporting ...................................................................... Patch Set 1: (3 comments) http://gerrit.cloudera.org:8080/#/c/8642/1/src/kudu/integration-tests/ts_tablet_manager-itest.cc File src/kudu/integration-tests/ts_tablet_manager-itest.cc: http://gerrit.cloudera.org:8080/#/c/8642/1/src/kudu/integration-tests/ts_tablet_manager-itest.cc@362 PS1, Line 362: reports > I see default param as nullptr as documentation that the arg is optional bu Ah, I see. That's appro http://gerrit.cloudera.org:8080/#/c/8642/1/src/kudu/integration-tests/ts_tablet_manager-itest.cc@476 PS1, Line 476: // The scenario below assumes the leader replica does not change anymore. : FLAGS_enable_leader_failure_detection = false; > I'm not sure it's truly safe but maybe it's safe enough for this kind of te I meant if the assertions for the status fail, then we need to retry the whole scenario with the state change, in my understanding -- putting the only code below would not help much in that case. That would require to put under ASSERT_EVENTUALLY almost all the scenario, which could hide some bugs. I would prefer to keep it as it is or just rewrite it, disabling leader failure detection in the very beginning of the test. Let's choose the first option for a while, if you accept it's more or less safe for this type of test. http://gerrit.cloudera.org:8080/#/c/8642/1/src/kudu/integration-tests/ts_tablet_manager-itest.cc@489 PS1, Line 489: const HealthReportPB& report(p.health_report()); > I don't think that's true. The copy constructor will get invoked and the in ok, we talked about that offline, and it seems the code below can clarify a bit on this (compile with -O0 to disable optimizations): #include <iostream> using namespace std; class A { public: A() : a_(0) { cout << "A::A()" << endl; } A(const A& a) { a_ = a.a_; cout << "A::A(&A)" << endl; } private: int a_; }; int main() { A a; const A& b = a; const A& c(a); A& d(a); return 0; } -- To view, visit http://gerrit.cloudera.org:8080/8642 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie62b49efebad9a123eec51dd302e375e46e0682d Gerrit-Change-Number: 8642 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Comment-Date: Mon, 27 Nov 2017 06:27:38 +0000 Gerrit-HasComments: Yes
