[kudu-CR] [java-client] Re-enable multi-master tests
Jean-Daniel Cryans has posted comments on this change. Change subject: [java-client] Re-enable multi-master tests .. Patch Set 4: -Verified Actually, holding off the +1 so that we don't push this patch before the other patches. -- To view, visit http://gerrit.cloudera.org:8080/3654 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia1051222738c84ef3d3e1a33b4981bc9454b7972 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jean-Daniel Cryans Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-HasComments: No
[kudu-CR] [java-client] Re-enable multi-master tests
Jean-Daniel Cryans has posted comments on this change. Change subject: [java-client] Re-enable multi-master tests .. Patch Set 4: Verified+1 ClientStressTest.TestStartScans is flaky. -- To view, visit http://gerrit.cloudera.org:8080/3654 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia1051222738c84ef3d3e1a33b4981bc9454b7972 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jean-Daniel Cryans Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-HasComments: No
[kudu-CR] [java-client] Re-enable multi-master tests
Adar Dembo has posted comments on this change. Change subject: [java-client] Re-enable multi-master tests .. Patch Set 4: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/3654 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia1051222738c84ef3d3e1a33b4981bc9454b7972 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jean-Daniel Cryans Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR] [java-client] Re-enable multi-master tests
Kudu Jenkins has posted comments on this change. Change subject: [java-client] Re-enable multi-master tests .. Patch Set 4: Build Started http://104.196.14.100/job/kudu-gerrit/2558/ -- To view, visit http://gerrit.cloudera.org:8080/3654 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia1051222738c84ef3d3e1a33b4981bc9454b7972 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jean-Daniel Cryans Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR] [java-client] Re-enable multi-master tests
Jean-Daniel Cryans has uploaded a new patch set (#4). Change subject: [java-client] Re-enable multi-master tests .. [java-client] Re-enable multi-master tests This patch makes TestMasterFailover useful again. It also adds the killing of masters to ITClient. Finally, it sets the raft heartbeat lower so that we don't wait 1.5s for leader elections. TestGetMasterRegistrationReceived was added since a previous version of this patch encountered a bug that such a simple unit test can detect. Change-Id: Ia1051222738c84ef3d3e1a33b4981bc9454b7972 --- M java/kudu-client/src/main/java/org/kududb/client/GetMasterRegistrationReceived.java M java/kudu-client/src/test/java/org/kududb/client/BaseKuduTest.java M java/kudu-client/src/test/java/org/kududb/client/ITClient.java M java/kudu-client/src/test/java/org/kududb/client/MiniKuduCluster.java A java/kudu-client/src/test/java/org/kududb/client/TestGetMasterRegistrationReceived.java M java/kudu-client/src/test/java/org/kududb/client/TestMasterFailover.java 6 files changed, 233 insertions(+), 23 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/54/3654/4 -- To view, visit http://gerrit.cloudera.org:8080/3654 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ia1051222738c84ef3d3e1a33b4981bc9454b7972 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jean-Daniel Cryans Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Jean-Daniel Cryans
[kudu-CR] [java-client] Re-enable multi-master tests
Jean-Daniel Cryans has posted comments on this change. Change subject: [java-client] Re-enable multi-master tests .. Patch Set 3: (6 comments) http://gerrit.cloudera.org:8080/#/c/3654/3/java/kudu-client/src/main/java/org/kududb/client/GetMasterRegistrationReceived.java File java/kudu-client/src/main/java/org/kududb/client/GetMasterRegistrationReceived.java: PS3, Line 109: and : // there's as many of them as responses received. > This much is obvious from the code itself, but what's not obvious is the si Good idea. http://gerrit.cloudera.org:8080/#/c/3654/3/java/kudu-client/src/test/java/org/kududb/client/BaseKuduTest.java File java/kudu-client/src/test/java/org/kududb/client/BaseKuduTest.java: Line 358: waitForAllTabletServers(); > I've been working on a change that'll allow ListTabletServers to be called I had a stale server-side compilation, with your new code it works. Sorry! http://gerrit.cloudera.org:8080/#/c/3654/3/java/kudu-client/src/test/java/org/kududb/client/TestGetMasterRegistrationReceived.java File java/kudu-client/src/test/java/org/kududb/client/TestGetMasterRegistrationReceived.java: Line 91: // Permutation of the previous > Nit: terminate with a period. Below too. Done Line 162: GetMasterRegistrationReceived grrm = new GetMasterRegistrationReceived(MASTERS, d); > Nice variable name. Thanks! :D Line 177: d.join(1000); // Don't care about the response. > Is there any significance to the timeout value here? Could we call join() w I forgot to remove it after adding a timeout on the test itself. Line 188: // Helper method that determines if the callback or errback should be called. > This one and makeGMRR() can be static. Done -- To view, visit http://gerrit.cloudera.org:8080/3654 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia1051222738c84ef3d3e1a33b4981bc9454b7972 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jean-Daniel Cryans Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-HasComments: Yes
[kudu-CR] [java-client] Re-enable multi-master tests
Adar Dembo has posted comments on this change. Change subject: [java-client] Re-enable multi-master tests .. Patch Set 3: (6 comments) http://gerrit.cloudera.org:8080/#/c/3654/3/java/kudu-client/src/main/java/org/kududb/client/GetMasterRegistrationReceived.java File java/kudu-client/src/main/java/org/kududb/client/GetMasterRegistrationReceived.java: PS3, Line 109: and : // there's as many of them as responses received. This much is obvious from the code itself, but what's not obvious is the significance. Could you modify the comment to explain why this matters? http://gerrit.cloudera.org:8080/#/c/3654/3/java/kudu-client/src/test/java/org/kududb/client/BaseKuduTest.java File java/kudu-client/src/test/java/org/kududb/client/BaseKuduTest.java: Line 358: waitForAllTabletServers(); I've been working on a change that'll allow ListTabletServers to be called on follower masters, as that reflects the new world order, where all masters receive heartbeats in order to keep their TS caches warm. With that change in place, I don't think waitForAllTabletServers() can be used as a proxy for "wait for a new master to be elected and for tservers to report to it". getTablesList() comes close (because it can only succeed after a new leader master is elected) but that's still not the same thing. Why do you actually need to wait? AFAICT killMasterLeader() is only used in TestMasterFailover.testKillLeader(), and the operations that follow the call should all succeed even without waiting, because the client itself is supposed to wait. The only exception is the createTable(), but that will be fixed with my patches. http://gerrit.cloudera.org:8080/#/c/3654/3/java/kudu-client/src/test/java/org/kududb/client/TestGetMasterRegistrationReceived.java File java/kudu-client/src/test/java/org/kududb/client/TestGetMasterRegistrationReceived.java: Line 91: // Permutation of the previous Nit: terminate with a period. Below too. Line 162: GetMasterRegistrationReceived grrm = new GetMasterRegistrationReceived(MASTERS, d); Nice variable name. Line 177: d.join(1000); // Don't care about the response. Is there any significance to the timeout value here? Could we call join() without a timeout? Line 188: // Helper method that determines if the callback or errback should be called. This one and makeGMRR() can be static. -- To view, visit http://gerrit.cloudera.org:8080/3654 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia1051222738c84ef3d3e1a33b4981bc9454b7972 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jean-Daniel Cryans Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-HasComments: Yes
[kudu-CR] [java-client] Re-enable multi-master tests
Jean-Daniel Cryans has posted comments on this change. Change subject: [java-client] Re-enable multi-master tests .. Patch Set 3: Verified+1 So much flakiness right now on jenkins. -- To view, visit http://gerrit.cloudera.org:8080/3654 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia1051222738c84ef3d3e1a33b4981bc9454b7972 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jean-Daniel Cryans Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-HasComments: No
[kudu-CR] [java-client] Re-enable multi-master tests
Kudu Jenkins has posted comments on this change. Change subject: [java-client] Re-enable multi-master tests .. Patch Set 3: Build Started http://104.196.14.100/job/kudu-gerrit/2545/ -- To view, visit http://gerrit.cloudera.org:8080/3654 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia1051222738c84ef3d3e1a33b4981bc9454b7972 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jean-Daniel Cryans Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR] [java-client] Re-enable multi-master tests
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/3654 to look at the new patch set (#3). Change subject: [java-client] Re-enable multi-master tests .. [java-client] Re-enable multi-master tests This patch makes TestMasterFailover useful again. It also adds the killing of masters to ITClient. Finally, it sets the raft heartbeat lower so that we don't wait 1.5s for leader elections. TestGetMasterRegistrationReceived was added since a previous version of this patch encountered a bug that such a simple unit test can detect. Change-Id: Ia1051222738c84ef3d3e1a33b4981bc9454b7972 --- M java/kudu-client/src/main/java/org/kududb/client/GetMasterRegistrationReceived.java M java/kudu-client/src/test/java/org/kududb/client/BaseKuduTest.java M java/kudu-client/src/test/java/org/kududb/client/ITClient.java M java/kudu-client/src/test/java/org/kududb/client/MiniKuduCluster.java A java/kudu-client/src/test/java/org/kududb/client/TestGetMasterRegistrationReceived.java M java/kudu-client/src/test/java/org/kududb/client/TestMasterFailover.java 6 files changed, 243 insertions(+), 28 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/54/3654/3 -- To view, visit http://gerrit.cloudera.org:8080/3654 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ia1051222738c84ef3d3e1a33b4981bc9454b7972 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jean-Daniel Cryans Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins
