Mike Percy has posted comments on this change. Change subject: KUDU-2033 (part 1). Add write and stop/start in a loop to TestLeaderFailover ......................................................................
Patch Set 4: (24 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 PS4, Line 2: client Seems more appropriate to put these in the "util" package instead of in "client". Line 4: don't skip 2 blank lines, only 1 PS4, Line 8: BooleanPredicate nit: rename to BooleanExpression. Line 16: long timeoutMillis) style: it is not required, but to be consistent with the rest of the Kudu code base I would suggestion indenting "long" to line up with "String" on the previous line. Line 17: throws Exception { style: indentation is off by 1. However I would suggest merging this line with the previous line. 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: style: don't leave multiple blank lines, only 1 PS4, Line 34: count nit: rowCount PS4, Line 34: { style: leave space before curly brace PS4, Line 34: scanUntilCount perhaps rename this to waitUntilRowCount() and allow the user to pass in timeoutMs instead of hard-coding Line 35: final int countCopy = count; specify "final int count" in the method signature instead Line 36: final KuduTable tableCopy = table; No need to do this, specify "final KuduTable table" in the method signature above. PS4, Line 37: AssertHelpers.assertEventuallyTrue use import static AssertHelpers.assertEventuallyTrue up top to shorten this Line 38: new AssertHelpers.BooleanPredicate() { use "import AssertHelpers.BooleanPredicate" above to shorten this. Line 39: @Override The indentation is off in this whole method (it should be 2 char indent, not 3) however also this part of code should also be indented 2 characters further right than "new BooleanPredicate" above. PS4, Line 45: 5000 Allow user to pass this in, however 5 sec is too short in extreme cases. Use 30 sec. Line 46: style: unnecessary blank line Line 48: /** style: leave a blank space between functions PS4, Line 50: restarting grammar: restarts PS4, Line 64: INNER_ROWS nit: What do you think about naming this variable something a little more descriptive, perhaps ROWS_PER_ITERATION? PS4, Line 65: OUTER_LOOP nit: OUTER_LOOP is a very mechanical name, and not really helpful for understanding the purpose of the code. How about naming this NUM_ITERATIONS instead? PS4, Line 68: START_ROWS I think we can just use INNER_ROWS for this one, too. Maybe just get rid of START_ROWS since I'm not sure it adds much value. Line 72: scanUntilCount(table, START_ROWS); Add comment that we have to potentially retry when scanning, which is why we use this helper function, because we have not enabled read-your-writes mode on this test. Line 76: Integer leaderPort = killTabletLeader(table); I think this would be a little less awkward as two steps: List<LocatedTablet> tablets = table.getTabletsLocations(DEFAULT_SLEEP); assertEquals(1, tablets.size()); int leaderPort = findLeaderTabletServerPort(tablets.get(0)); killTabletServerOnPort(leaderPort); Also, then you don't have to change the return types to return ports and stuff. -- 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: 4 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
