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 <jdcry...@apache.org>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jdcry...@apache.org>
Gerrit-HasComments: Yes

Reply via email to