Edward Fancher has posted comments on this change. Change subject: KUDU-2033 (part 1). Add write and stop/start in a loop to TestLeaderFailover ......................................................................
Patch Set 5: (22 comments) http://gerrit.cloudera.org:8080/#/c/7456/4/java/kudu-client/src/test/java/org/apache/kudu/client/AssertHelpers.java File java/kudu-client/src/test/java/org/apache/kudu/client/AssertHelpers.java: Line 1 > Needs ASL 2.0 license header at the top of the file Done PS4, Line 2: > Seems more appropriate to put these in the "util" package instead of in "cl Done Line 4 > don't skip 2 blank lines, only 1 Done PS4, Line 8: > nit: rename to BooleanExpression. Done Line 16 > style: it is not required, but to be consistent with the rest of the Kudu c Done Line 17 > style: indentation is off by 1. However I would suggest merging this line w Done http://gerrit.cloudera.org:8080/#/c/7456/4/java/kudu-client/src/test/java/org/apache/kudu/client/TestMultipleLeaderFailover.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestMultipleLeaderFailover.java: Line 31: public static void setUpBeforeClass() throws Exception { > style: don't leave multiple blank lines, only 1 Done PS4, Line 34: > style: leave space before curly brace Done PS4, Line 34: > perhaps rename this to waitUntilRowCount() and allow the user to pass in ti Done PS4, Line 34: > nit: rowCount Done Line 35: private void waitUntilRowCount(final KuduTable table, final int rowCount, int timeoutMs) > specify "final int count" in the method signature instead Done Line 36: throws Exception { > No need to do this, specify "final KuduTable table" in the method signature Done Line 38: new BooleanExpression() { > use "import AssertHelpers.BooleanPredicate" above to shorten this. Done Line 39: @Override > The indentation is off in this whole method (it should be 2 char indent, no Ok, I redid it with intellij's settings. Does it seem correct now? PS4, Line 45: }, > Allow user to pass this in, however 5 sec is too short in extreme cases. Us Done Line 46: } > style: unnecessary blank line Done Line 48: /** > style: leave a blank space between functions Done PS4, Line 50: restarts t > grammar: restarts Done PS4, Line 65: TOTAL_ROWS > nit: OUTER_LOOP is a very mechanical name, and not really helpful for under Done PS4, Line 68: teBasicSch > I think we can just use INNER_ROWS for this one, too. Maybe just get rid of Done Line 72: > Add comment that we have to potentially retry when scanning, which is why w Done Line 76: assertEquals(1, tablets.size()); > I think this would be a little less awkward as two steps: Done -- To view, visit http://gerrit.cloudera.org:8080/7456 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie93a15084a4c4c0748dc74c005b8313f443a5ba9 Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Edward Fancher <[email protected]> Gerrit-Reviewer: Edward Fancher <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
