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 2017/11/24 07:36:48 UTC
[kudu-CR] KUDU-1097 (patch1): test for replica health reporting
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/8642
Change subject: KUDU-1097 (patch1): test for replica health reporting
......................................................................
KUDU-1097 (patch1): test for replica health reporting
Added a test to verify that the leader tablet replica reports on the
replica health changes. The tests verifies that the health reports
are present in the Raft consensus state reported by the leader replica.
The test also verifies that the incremental tablet reports contain
appropriate information once the replica health status changes.
Change-Id: Ie62b49efebad9a123eec51dd302e375e46e0682d
---
M src/kudu/integration-tests/ts_tablet_manager-itest.cc
1 file changed, 278 insertions(+), 49 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/42/8642/1
--
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: newchange
Gerrit-Change-Id: Ie62b49efebad9a123eec51dd302e375e46e0682d
Gerrit-Change-Number: 8642
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] KUDU-1097 (patch1): test for replica health reporting
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Mon, 27 Nov 2017 06:27:38 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1097 (patch1): test for replica health reporting
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/8642 )
Change subject: KUDU-1097 (patch1): test for replica health reporting
......................................................................
KUDU-1097 (patch1): test for replica health reporting
Added a test to verify that the leader tablet replica reports on the
replica health changes. The tests verifies that the health reports
are present in the Raft consensus state reported by the leader replica.
The test also verifies that the incremental tablet reports contain
appropriate information once the replica health status changes.
Change-Id: Ie62b49efebad9a123eec51dd302e375e46e0682d
Reviewed-on: http://gerrit.cloudera.org:8080/8642
Tested-by: Kudu Jenkins
Reviewed-by: Mike Percy <mp...@apache.org>
---
M src/kudu/integration-tests/ts_tablet_manager-itest.cc
1 file changed, 291 insertions(+), 49 deletions(-)
Approvals:
Kudu Jenkins: Verified
Mike Percy: Looks good to me, approved
--
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: merged
Gerrit-Change-Id: Ie62b49efebad9a123eec51dd302e375e46e0682d
Gerrit-Change-Number: 8642
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] KUDU-1097 (patch1): test for replica health reporting
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8642
to look at the new patch set (#2).
Change subject: KUDU-1097 (patch1): test for replica health reporting
......................................................................
KUDU-1097 (patch1): test for replica health reporting
Added a test to verify that the leader tablet replica reports on the
replica health changes. The tests verifies that the health reports
are present in the Raft consensus state reported by the leader replica.
The test also verifies that the incremental tablet reports contain
appropriate information once the replica health status changes.
Change-Id: Ie62b49efebad9a123eec51dd302e375e46e0682d
---
M src/kudu/integration-tests/ts_tablet_manager-itest.cc
1 file changed, 291 insertions(+), 49 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/42/8642/2
--
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: newpatchset
Gerrit-Change-Id: Ie62b49efebad9a123eec51dd302e375e46e0682d
Gerrit-Change-Number: 8642
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] KUDU-1097 (patch1): test for replica health reporting
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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:
(1 comment)
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
> Ah, I see. That's appro
I meant that's an interesting approach. Let me put that then for the documentation purposes.
--
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 <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Mon, 27 Nov 2017 06:29:41 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1097 (patch1): test for replica health reporting
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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:
(17 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@124
PS1, Line 124: Status PrepareTabletReplicas(MonoDelta timeout,
> nit: doc these functions
Done
http://gerrit.cloudera.org:8080/#/c/8642/1/src/kudu/integration-tests/ts_tablet_manager-itest.cc@362
PS1, Line 362: auto get_health_reports = [&](map<string, HealthReportPB>* reports,
> nit: doc this closure
Done
http://gerrit.cloudera.org:8080/#/c/8642/1/src/kudu/integration-tests/ts_tablet_manager-itest.cc@362
PS1, Line 362: reports
> nit: if this is optional, how about we default it to nullptr
This does not make much sense because this closure is never invoked with two parameters set to the default values.
http://gerrit.cloudera.org:8080/#/c/8642/1/src/kudu/integration-tests/ts_tablet_manager-itest.cc@366
PS1, Line 366: e
> nit: usually when we use "e" when iterating it's for an entry in a map, but
Done
http://gerrit.cloudera.org:8080/#/c/8642/1/src/kudu/integration-tests/ts_tablet_manager-itest.cc@367
PS1, Line 367: con
> nit: mind spelling out "consensus" here?
Done
http://gerrit.cloudera.org:8080/#/c/8642/1/src/kudu/integration-tests/ts_tablet_manager-itest.cc@369
PS1, Line 369: cs.has_leader_uuid()
> nit: no need to check has_leader_uuid(), since if it's not set it will retu
Done
http://gerrit.cloudera.org:8080/#/c/8642/1/src/kudu/integration-tests/ts_tablet_manager-itest.cc@378
PS1, Line 378: config
> nit: perhaps name this committed_config as to avoid any confusion about whi
This is just a local shortcut for the next 10 lines, so I think this should not bring a confusion -- it's easy to see what the 'config' is.
http://gerrit.cloudera.org:8080/#/c/8642/1/src/kudu/integration-tests/ts_tablet_manager-itest.cc@392
PS1, Line 392: auto get_committed_config_from_reports = [&](const string& leader_replica_uuid,
> nit: doc this closure
Done
http://gerrit.cloudera.org:8080/#/c/8642/1/src/kudu/integration-tests/ts_tablet_manager-itest.cc@409
PS1, Line 409: ;
> typo
Done
http://gerrit.cloudera.org:8080/#/c/8642/1/src/kudu/integration-tests/ts_tablet_manager-itest.cc@410
PS1, Line 410: const TabletReportPB& report = reports[0];
: ASSERT_EQ(1, reports.size());
> the order of these lines should be reversed
good catch!
http://gerrit.cloudera.org:8080/#/c/8642/1/src/kudu/integration-tests/ts_tablet_manager-itest.cc@432
PS1, Line 432: EXPECT_EQ
> EXPECT_EQ will not trigger an ASSERT_EVENTUALLY failure; consider using ASS
Good catch! Exactly: EXPECT_XX macros do not generate fatal test failures. It seems I just moved those under the ASSERT_EVENTUALLY, not thinking about that.
http://gerrit.cloudera.org:8080/#/c/8642/1/src/kudu/integration-tests/ts_tablet_manager-itest.cc@469
PS1, Line 469: EXPECT_EQ
> ASSERT_EQ
Done
http://gerrit.cloudera.org:8080/#/c/8642/1/src/kudu/integration-tests/ts_tablet_manager-itest.cc@471
PS1, Line 471: EXPECT_EQ
> ASSERT_EQ
Done
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;
> This isn't set specified as a runtime flag and i'm not sure it's really saf
I saw that's how it's done in TsTabletManagerITest::TestReportNewLeaderOnLeaderChange above -- they call this after starting the cluster already. So, I assumed it was safe enough.
I also looked into the code, and it seemed safe to change this in run-time. Probably, that's because it has changed recently: we don't use dedicated thread for that anymore, so toggling this is not error-prone anymore.
Wrapping the code below in ASSERT_EVENTUALLY would be brittle, IMO.
http://gerrit.cloudera.org:8080/#/c/8642/1/src/kudu/integration-tests/ts_tablet_manager-itest.cc@485
PS1, Line 485: p
> nit: mind just spelling out "peer" here?
Done
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());
> nit: why copy constructor? why not just get a ref like:
It's a constructor for a reference. There is no difference here -- in both cases it will be a reference, no copy constructor should be invoked. It's just one symbol less.
http://gerrit.cloudera.org:8080/#/c/8642/1/src/kudu/integration-tests/ts_tablet_manager-itest.cc@491
PS1, Line 491: EXPECT_EQ
> nit: if we wrap this in ASSERT_EVENTUALLY then we will have to change this
I think we can get away with changing the 'enable_leader_failure_detection' flag in run-time, no?
--
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 <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Mon, 27 Nov 2017 05:26:13 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1097 (patch1): test for replica health reporting
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/8642 )
Change subject: KUDU-1097 (patch1): test for replica health reporting
......................................................................
Patch Set 2:
(1 comment)
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@489
PS1, Line 489: he scenario below assumes the leader replica doe
> I don't think that's true. The copy constructor will get invoked and the in
OK, apparently you were right :)
--
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: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Mon, 27 Nov 2017 06:15:01 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1097 (patch1): test for replica health reporting
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy 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:
(17 comments)
Looks good. Thanks for writing this test for the patch 1 functionality. I have some nitpicky feedback and that's about it.
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@124
PS1, Line 124: Status PrepareTabletReplicas(MonoDelta timeout,
nit: doc these functions
http://gerrit.cloudera.org:8080/#/c/8642/1/src/kudu/integration-tests/ts_tablet_manager-itest.cc@362
PS1, Line 362: auto get_health_reports = [&](map<string, HealthReportPB>* reports,
nit: doc this closure
http://gerrit.cloudera.org:8080/#/c/8642/1/src/kudu/integration-tests/ts_tablet_manager-itest.cc@362
PS1, Line 362: reports
nit: if this is optional, how about we default it to nullptr
http://gerrit.cloudera.org:8080/#/c/8642/1/src/kudu/integration-tests/ts_tablet_manager-itest.cc@366
PS1, Line 366: e
nit: usually when we use "e" when iterating it's for an entry in a map, but this container is just a vector, so we should just use "replica" or something similar for this variable name.
http://gerrit.cloudera.org:8080/#/c/8642/1/src/kudu/integration-tests/ts_tablet_manager-itest.cc@367
PS1, Line 367: con
nit: mind spelling out "consensus" here?
http://gerrit.cloudera.org:8080/#/c/8642/1/src/kudu/integration-tests/ts_tablet_manager-itest.cc@369
PS1, Line 369: cs.has_leader_uuid()
nit: no need to check has_leader_uuid(), since if it's not set it will return the default instance which is an empty string and thus won't match peer_uuid()
http://gerrit.cloudera.org:8080/#/c/8642/1/src/kudu/integration-tests/ts_tablet_manager-itest.cc@378
PS1, Line 378: config
nit: perhaps name this committed_config as to avoid any confusion about which config it is
http://gerrit.cloudera.org:8080/#/c/8642/1/src/kudu/integration-tests/ts_tablet_manager-itest.cc@392
PS1, Line 392: auto get_committed_config_from_reports = [&](const string& leader_replica_uuid,
nit: doc this closure
http://gerrit.cloudera.org:8080/#/c/8642/1/src/kudu/integration-tests/ts_tablet_manager-itest.cc@409
PS1, Line 409: ;
typo
http://gerrit.cloudera.org:8080/#/c/8642/1/src/kudu/integration-tests/ts_tablet_manager-itest.cc@410
PS1, Line 410: const TabletReportPB& report = reports[0];
: ASSERT_EQ(1, reports.size());
the order of these lines should be reversed
http://gerrit.cloudera.org:8080/#/c/8642/1/src/kudu/integration-tests/ts_tablet_manager-itest.cc@432
PS1, Line 432: EXPECT_EQ
EXPECT_EQ will not trigger an ASSERT_EVENTUALLY failure; consider using ASSERT_EQ here
http://gerrit.cloudera.org:8080/#/c/8642/1/src/kudu/integration-tests/ts_tablet_manager-itest.cc@469
PS1, Line 469: EXPECT_EQ
ASSERT_EQ
http://gerrit.cloudera.org:8080/#/c/8642/1/src/kudu/integration-tests/ts_tablet_manager-itest.cc@471
PS1, Line 471: EXPECT_EQ
ASSERT_EQ
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;
This isn't set specified as a runtime flag and i'm not sure it's really safe to change at runtime.
Something we often to do in these situations is just wrap an ASSERT_EVENTUALLY() around any multi-step test sections.
http://gerrit.cloudera.org:8080/#/c/8642/1/src/kudu/integration-tests/ts_tablet_manager-itest.cc@485
PS1, Line 485: p
nit: mind just spelling out "peer" here?
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());
nit: why copy constructor? why not just get a ref like:
const HealthReportPB& report = p.health_report();
http://gerrit.cloudera.org:8080/#/c/8642/1/src/kudu/integration-tests/ts_tablet_manager-itest.cc@491
PS1, Line 491: EXPECT_EQ
nit: if we wrap this in ASSERT_EVENTUALLY then we will have to change this one and the one below it to ASSERT_EQ
--
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 <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Mon, 27 Nov 2017 03:21:43 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1097 (patch1): test for replica health reporting
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/8642 )
Change subject: KUDU-1097 (patch1): test for replica health reporting
......................................................................
Patch Set 2:
I'm ok with this, let's push it
--
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: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Mon, 27 Nov 2017 06:29:17 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1097 (patch1): test for replica health reporting
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/8642 )
Change subject: KUDU-1097 (patch1): test for replica health reporting
......................................................................
Patch Set 2: Code-Review+2
(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: eports.
> This does not make much sense because this closure is never invoked with tw
I see default param as nullptr as documentation that the arg is optional but I'm ok with this
http://gerrit.cloudera.org:8080/#/c/8642/1/src/kudu/integration-tests/ts_tablet_manager-itest.cc@476
PS1, Line 476: map<string, HealthReportPB> reports;
: NO_FATALS(get_health_reports(&reports));
> I saw that's how it's done in TsTabletManagerITest::TestReportNewLeaderOnLe
I'm not sure it's truly safe but maybe it's safe enough for this kind of test... I don't see how ASSERT_EVENTUALLY would be brittle but I can live with it
http://gerrit.cloudera.org:8080/#/c/8642/1/src/kudu/integration-tests/ts_tablet_manager-itest.cc@489
PS1, Line 489: he scenario below assumes the leader replica doe
> It's a constructor for a reference. There is no difference here -- in both
I don't think that's true. The copy constructor will get invoked and the instance will be assigned to a temporary and "report" will act as a reference to that temporary. Actually I guess this part of C++ is a bit magical for me. const string& s = "blah" should act the same way.
That said, it's a test and not a very big deal.
--
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: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Mon, 27 Nov 2017 05:58:27 +0000
Gerrit-HasComments: Yes