Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/8393 )
Change subject: KUDU-2200: provide better diagnostics when connecting to a subset of masters ...................................................................... Patch Set 2: (4 comments) http://gerrit.cloudera.org:8080/#/c/8393/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectToCluster.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectToCluster.java: http://gerrit.cloudera.org:8080/#/c/8393/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectToCluster.java@108 PS1, Line 108: // One of the connections should have succeeded. : assertEquals(1, successes); > Yah I'm just being kind of conservative. If the client always hard fails w I see. For some reason I thought this patch was to introduce a stricter policy for specifying master end-points, but it seems it's more about proper error reporting and actionable message in that case. I think it makes sense as well. Thanks for clarifying. http://gerrit.cloudera.org:8080/#/c/8393/2/src/kudu/integration-tests/master_replication-itest.cc File src/kudu/integration-tests/master_replication-itest.cc: http://gerrit.cloudera.org:8080/#/c/8393/2/src/kudu/integration-tests/master_replication-itest.cc@308 PS2, Line 308: LOG(ERROR) << resp.DebugString(); nit: is it crucial to output this, especially as an error? http://gerrit.cloudera.org:8080/#/c/8393/2/src/kudu/integration-tests/master_replication-itest.cc@333 PS2, Line 333: // - 0, in the case that no master had elected itself yet : // - 1, in the case that one master had become leader by the time we connected. Is it possible that leader re-election happens among masters just between clients are connected and successes becomes 2? Most likely, it's not the case for 'fast' builds, but a TSAN build might be the case. http://gerrit.cloudera.org:8080/#/c/8393/2/src/kudu/integration-tests/master_replication-itest.cc@335 PS2, Line 335: LE It seems it should be EXPECT_GE(). Maybe, it's worth adding the same relaxed GE assert into the Java test as well? There I saw strict EQ assert. -- To view, visit http://gerrit.cloudera.org:8080/8393 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I52f903e1aa5ae6948ca1ba6d4d856c3c9dc73d56 Gerrit-Change-Number: 8393 Gerrit-PatchSet: 2 Gerrit-Owner: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Tue, 31 Oct 2017 06:19:25 +0000 Gerrit-HasComments: Yes
