[kudu-CR] KUDU-2033 (part 1). Add write and stop/start in a loop to TestLeaderFailover
Mike Percy has submitted this change and it was merged. Change subject: KUDU-2033 (part 1). Add write and stop/start in a loop to TestLeaderFailover .. KUDU-2033 (part 1). Add write and stop/start in a loop to TestLeaderFailover TestLeaderFailover is made more robust by adding a write loop in which the tablet leader is killed and restarted. The main idea is to check that the writes complete and can be read after the leader is killed. Done in a loop so we can be sure that we stay stable through multiple stops and restarts of the leader tablet. Change-Id: Ie93a15084a4c4c0748dc74c005b8313f443a5ba9 Reviewed-on: http://gerrit.cloudera.org:8080/7456 Reviewed-by: Mike Percy Tested-by: Mike Percy --- M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java A java/kudu-client/src/test/java/org/apache/kudu/client/TestMultipleLeaderFailover.java A java/kudu-client/src/test/java/org/apache/kudu/util/AssertHelpers.java 3 files changed, 137 insertions(+), 1 deletion(-) Approvals: Mike Percy: Looks good to me, approved; Verified -- To view, visit http://gerrit.cloudera.org:8080/7456 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: Ie93a15084a4c4c0748dc74c005b8313f443a5ba9 Gerrit-PatchSet: 9 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Edward Fancher Gerrit-Reviewer: Edward Fancher Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2033 (part 1). Add write and stop/start in a loop to TestLeaderFailover
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 8: Apparently raft_consensus-itest is flaky under TSAN. Overriding unrelated test failure. -- 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: 8 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Edward Fancher Gerrit-Reviewer: Edward Fancher Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] KUDU-2033 (part 1). Add write and stop/start in a loop to TestLeaderFailover
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 8: Verified+1 -- 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: 8 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Edward Fancher Gerrit-Reviewer: Edward Fancher Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] KUDU-2033 (part 1). Add write and stop/start in a loop to TestLeaderFailover
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 8: Code-Review+2 -- 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: 8 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Edward Fancher Gerrit-Reviewer: Edward Fancher Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] KUDU-2033 (part 1). Add write and stop/start in a loop to TestLeaderFailover
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 8: (1 comment) http://gerrit.cloudera.org:8080/#/c/7456/7/java/kudu-client/src/test/java/org/apache/kudu/util/AssertHelpers.java File java/kudu-client/src/test/java/org/apache/kudu/util/AssertHelpers.java: PS7, Line 30: long deadlineNanos = System.nanoTime() + timeoutMillis * 100; : boolean success; : : do { : success = expression.get(); : if (success) break; : > How about using a do-while loop so you only have to call .get() once in the 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: 8 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Edward Fancher Gerrit-Reviewer: Edward Fancher Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] KUDU-2033 (part 1). Add write and stop/start in a loop to TestLeaderFailover
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7456 to look at the new patch set (#8). Change subject: KUDU-2033 (part 1). Add write and stop/start in a loop to TestLeaderFailover .. KUDU-2033 (part 1). Add write and stop/start in a loop to TestLeaderFailover TestLeaderFailover is made more robust by adding a write loop in which the tablet leader is killed and restarted. The main idea is to check that the writes complete and can be read after the leader is killed. Done in a loop so we can be sure that we stay stable through multiple stops and restarts of the leader tablet. Change-Id: Ie93a15084a4c4c0748dc74c005b8313f443a5ba9 --- M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java A java/kudu-client/src/test/java/org/apache/kudu/client/TestMultipleLeaderFailover.java A java/kudu-client/src/test/java/org/apache/kudu/util/AssertHelpers.java 3 files changed, 137 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/7456/8 -- To view, visit http://gerrit.cloudera.org:8080/7456 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ie93a15084a4c4c0748dc74c005b8313f443a5ba9 Gerrit-PatchSet: 8 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Edward Fancher Gerrit-Reviewer: Edward Fancher Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2033 (part 1). Add write and stop/start in a loop to TestLeaderFailover
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 7: (1 comment) http://gerrit.cloudera.org:8080/#/c/7456/7/java/kudu-client/src/test/java/org/apache/kudu/util/AssertHelpers.java File java/kudu-client/src/test/java/org/apache/kudu/util/AssertHelpers.java: PS7, Line 30: long start = System.nanoTime(); : boolean success = expression.get(); : : while (!success && System.nanoTime() < start + timeoutMillis*1000) { : Thread.sleep(timeoutMillis / 10); : success = expression.get(); : } How about using a do-while loop so you only have to call .get() once in the code. Also, there is a problem with the nanos vs millis math. Consider naming the variables according to their granularity and doing something like: long deadlineNanos = System.nanoTime() + timeoutMillis * 100; boolean success; do { success = expression.get(); if (success) break; Thread.sleep(50); // Sleep for 50ns. } while (system.nanoTime() < deadlineNanos); Also I think the choice of timeoutMillis / 10 isn't great in the case that you have a long timeout (i.e. 30 sec) so maybe 50ms is a reasonable backoff period for most tests. -- 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: 7 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Edward Fancher Gerrit-Reviewer: Edward Fancher Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] KUDU-2033 (part 1). Add write and stop/start in a loop to TestLeaderFailover
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 6: (9 comments) http://gerrit.cloudera.org:8080/#/c/7456/5/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: PS5, Line 35: lon > it's typical to use a long for millisecond time values in Java Done PS5, Line 64: NUM_ITERAT > still named OUTER_LOOP, see comments on rev 4 Done PS5, Line 71: DEFAU > Better to not use magic numbers. For consistency with the rest of the clien Done PS5, Line 89: yet > no need to say "yet" here, RYW will always be optional Done PS5, Line 90: DEFAU > DEFAULT_SLEEP Done PS5, Line 93: DEFAU > use DEFAULT_SLEEP here and elsewhere Done http://gerrit.cloudera.org:8080/#/c/7456/5/java/kudu-client/src/test/java/org/apache/kudu/util/AssertHelpers.java File java/kudu-client/src/test/java/org/apache/kudu/util/AssertHelpers.java: PS5, Line 27: > This is a standard Java thing and not required to be mentioned here Done Line 28: public static void assertEventuallyTrue(String description, BooleanExpression expression, > No need to give credit for this. Flume is ASL 2.0 which doesn't require att Done PS5, Line 36: > would be nice to avoid calling 'get()' twice, in case it's expensive. For e Better? -- 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: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Edward Fancher Gerrit-Reviewer: Edward Fancher Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] KUDU-2033 (part 1). Add write and stop/start in a loop to TestLeaderFailover
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7456 to look at the new patch set (#7). Change subject: KUDU-2033 (part 1). Add write and stop/start in a loop to TestLeaderFailover .. KUDU-2033 (part 1). Add write and stop/start in a loop to TestLeaderFailover TestLeaderFailover is made more robust by adding a write loop in which the tablet leader is killed and restarted. The main idea is to check that the writes complete and can be read after the leader is killed. Done in a loop so we can be sure that we stay stable through multiple stops and restarts of the leader tablet. Change-Id: Ie93a15084a4c4c0748dc74c005b8313f443a5ba9 --- M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java A java/kudu-client/src/test/java/org/apache/kudu/client/TestMultipleLeaderFailover.java A java/kudu-client/src/test/java/org/apache/kudu/util/AssertHelpers.java 3 files changed, 136 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/7456/7 -- To view, visit http://gerrit.cloudera.org:8080/7456 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ie93a15084a4c4c0748dc74c005b8313f443a5ba9 Gerrit-PatchSet: 7 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Edward Fancher Gerrit-Reviewer: Edward Fancher Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2033 (part 1). Add write and stop/start in a loop to TestLeaderFailover
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7456 to look at the new patch set (#6). Change subject: KUDU-2033 (part 1). Add write and stop/start in a loop to TestLeaderFailover .. KUDU-2033 (part 1). Add write and stop/start in a loop to TestLeaderFailover TestLeaderFailover is made more robust by adding a write loop in which the tablet leader is killed and restarted. The main idea is to check that the writes complete and can be read after the leader is killed. Done in a loop so we can be sure that we stay stable through multiple stops and restarts of the leader tablet. Change-Id: Ie93a15084a4c4c0748dc74c005b8313f443a5ba9 --- M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java A java/kudu-client/src/test/java/org/apache/kudu/client/TestMultipleLeaderFailover.java A java/kudu-client/src/test/java/org/apache/kudu/util/AssertHelpers.java 3 files changed, 136 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/7456/6 -- To view, visit http://gerrit.cloudera.org:8080/7456 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ie93a15084a4c4c0748dc74c005b8313f443a5ba9 Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Edward Fancher Gerrit-Reviewer: Edward Fancher Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2033 (part 1). Add write and stop/start in a loop to TestLeaderFailover
Todd Lipcon 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: (1 comment) http://gerrit.cloudera.org:8080/#/c/7456/5/java/kudu-client/src/test/java/org/apache/kudu/util/AssertHelpers.java File java/kudu-client/src/test/java/org/apache/kudu/util/AssertHelpers.java: PS5, Line 36: expression.get would be nice to avoid calling 'get()' twice, in case it's expensive. For example the Hadoop implementation here: https://github.com/apache/hadoop/blob/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/test/GenericTestUtils.java#L353 also using monotonic time via a Stopwatch or using System.nanoTime is better than currentTimeMillis() since it won't get affected by time changes -- 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 Gerrit-Reviewer: Edward Fancher Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] KUDU-2033 (part 1). Add write and stop/start in a loop to TestLeaderFailover
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 5: (9 comments) looks good, just a couple minor things now 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 39: @Override > Ok, I redid it with intellij's settings. Does it seem correct now? lgtm now, thanks! http://gerrit.cloudera.org:8080/#/c/7456/5/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: PS5, Line 35: int it's typical to use a long for millisecond time values in Java PS5, Line 64: OUTER_LOOP still named OUTER_LOOP, see comments on rev 4 PS5, Line 71: 3 Better to not use magic numbers. For consistency with the rest of the client test codebase (even though it's a timeout, not a sleep), it seems best to use DEFAULT_SLEEP for the timeout value here and elsewhere. PS5, Line 89: yet no need to say "yet" here, RYW will always be optional PS5, Line 90: 3 DEFAULT_SLEEP PS5, Line 93: 3 use DEFAULT_SLEEP here and elsewhere http://gerrit.cloudera.org:8080/#/c/7456/5/java/kudu-client/src/test/java/org/apache/kudu/util/AssertHelpers.java File java/kudu-client/src/test/java/org/apache/kudu/util/AssertHelpers.java: PS5, Line 27: Note that any variables in BooleanExpression.get() will need to be final This is a standard Java thing and not required to be mentioned here Line 28: // Borrowed from Flume. No need to give credit for this. Flume is ASL 2.0 which doesn't require attribution, plus I originally wrote it and it's fine with me not to give attribution. -- 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 Gerrit-Reviewer: Edward Fancher Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] KUDU-2033 (part 1). Add write and stop/start in a loop to TestLeaderFailover
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 Gerrit-Reviewer: Edward Fancher Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] KUDU-2033 (part 1). Add write and stop/start in a loop to TestLeaderFailover
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7456 to look at the new patch set (#5). Change subject: KUDU-2033 (part 1). Add write and stop/start in a loop to TestLeaderFailover .. KUDU-2033 (part 1). Add write and stop/start in a loop to TestLeaderFailover TestLeaderFailover is made more robust by adding a write loop in which the tablet leader is killed and restarted. The main idea is to check that the writes complete and can be read after the leader is killed. Done in a loop so we can be sure that we stay stable through multiple stops and restarts of the leader tablet. Change-Id: Ie93a15084a4c4c0748dc74c005b8313f443a5ba9 --- M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java A java/kudu-client/src/test/java/org/apache/kudu/client/TestMultipleLeaderFailover.java A java/kudu-client/src/test/java/org/apache/kudu/util/AssertHelpers.java 3 files changed, 134 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/7456/5 -- To view, visit http://gerrit.cloudera.org:8080/7456 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ie93a15084a4c4c0748dc74c005b8313f443a5ba9 Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Edward Fancher Gerrit-Reviewer: Edward Fancher Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2033 (part 1). Add write and stop/start in a loop to TestLeaderFailover
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 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 Gerrit-Reviewer: Edward Fancher Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] KUDU-2033 (part 1). Add write and stop/start in a loop to TestLeaderFailover
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 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/7456/2/java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java File java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java: PS2, Line 351: int kil > Can we return an int instead of an Integer here and below? 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: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Edward Fancher Gerrit-Reviewer: Edward Fancher Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] KUDU-2033 (part 1). Add write and stop/start in a loop to TestLeaderFailover
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 4: (6 comments) http://gerrit.cloudera.org:8080/#/c/7456/2//COMMIT_MSG Commit Message: Line 7: KUDU-2033 (part 1). Add write and stop/start in a loop to TestLeaderFailover > nit: Avoid period at end of subject line in a commit message Fixed. http://gerrit.cloudera.org:8080/#/c/7456/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestLeaderFailover.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestLeaderFailover.java: Line 42:* This test writes 3 rows, kills the leader, then tries to write another 3 rows. Finally it > nit: line length should be <= 100 chars Done. Line 49: KuduSession session = syncClient.newSession(); > Can we keep the existing simple test and add an additional test that uses t Done Line 51: session.apply(createBasicSchemaInsert(table, i)); > style: use final and all caps for constants Done Line 67: scanner = client.newScannerBuilder(table).build(); > I find this logic a bit difficult to follow. I'd suggest keeping a counter Done Line 77 > these assertions should probably be assert eventually 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: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Edward Fancher Gerrit-Reviewer: Edward Fancher Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] KUDU-2033 (part 1). Add write and stop/start in a loop to TestLeaderFailover
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7456 to look at the new patch set (#4). Change subject: KUDU-2033 (part 1). Add write and stop/start in a loop to TestLeaderFailover .. KUDU-2033 (part 1). Add write and stop/start in a loop to TestLeaderFailover TestLeaderFailover is made more robust by adding a write loop in which the tablet leader is killed and restarted. The main idea is to check that the writes complete and can be read after the leader is killed. Done in a loop so we can be sure that we stay stable through multiple stops and restarts of the leader tablet. Change-Id: Ie93a15084a4c4c0748dc74c005b8313f443a5ba9 --- A java/kudu-client/src/test/java/org/apache/kudu/client/AssertHelpers.java M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java A java/kudu-client/src/test/java/org/apache/kudu/client/TestMultipleLeaderFailover.java 3 files changed, 124 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/7456/4 -- To view, visit http://gerrit.cloudera.org:8080/7456 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ie93a15084a4c4c0748dc74c005b8313f443a5ba9 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Edward Fancher Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2033 (part 1). Add write and stop/start in a loop to TestLeaderFailover
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7456 to look at the new patch set (#3). Change subject: KUDU-2033 (part 1). Add write and stop/start in a loop to TestLeaderFailover .. KUDU-2033 (part 1). Add write and stop/start in a loop to TestLeaderFailover TestLeaderFailover is made more robust by adding a write loop in which the tablet leader is killed and restarted. The main idea is to check that the writes complete and can be read after the leader is killed. Done in a loop so we can be sure that we stay stable through multiple stops and restarts of the leader tablet. Change-Id: Ie93a15084a4c4c0748dc74c005b8313f443a5ba9 --- A java/kudu-client/src/test/java/org/apache/kudu/client/AssertHelpers.java M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java A java/kudu-client/src/test/java/org/apache/kudu/client/TestMultipleLeaderFailover.java 3 files changed, 124 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/7456/3 -- To view, visit http://gerrit.cloudera.org:8080/7456 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ie93a15084a4c4c0748dc74c005b8313f443a5ba9 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Edward Fancher Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2033 (part 1). Add write and stop/start in a loop to TestLeaderFailover.
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 2: (7 comments) http://gerrit.cloudera.org:8080/#/c/7456/2//COMMIT_MSG Commit Message: Line 7: KUDU-2033 (part 1). Add write and stop/start in a loop to TestLeaderFailover. nit: Avoid period at end of subject line in a commit message http://gerrit.cloudera.org:8080/#/c/7456/2/java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java File java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java: PS2, Line 351: Integer Can we return an int instead of an Integer here and below? I can't see any reason to use a boxed primitive here. http://gerrit.cloudera.org:8080/#/c/7456/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestLeaderFailover.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestLeaderFailover.java: Line 42:* This test writes 3 rows. Then in a loop, it kills the leader, then tries to write inner_row rows. Verifying nit: line length should be <= 100 chars Line 49: public void testFailover() throws Exception { Can we keep the existing simple test and add an additional test that uses this "stress test" approach? Line 51: int startRows = 3; style: use final and all caps for constants Line 67: for (int j = i; j < i + innerRows; j++) { I find this logic a bit difficult to follow. I'd suggest keeping a counter like currentRows and just increment that each time you insert a row, then use simple i and j counters as needed to control the number of iterations per loop. something like: int currentRows = 0; for (int i = 0; i < startRows; i++) { session.apply(createBasicSchemaInsert(table, i)); currentRows++; } for (int i = 0; i < numIterations; i++) { // ... for (int j = 0; j < rowsPerIteration; j++) { session.apply(...); currentRows++; } } // ... assertEquals(totalRowsToInsert, countRowsInScan(scanner)); Line 77: assertEquals(i + innerRows, countRowsInScan(scanner)); these assertions should probably be assert eventually -- 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: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Edward Fancher Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-HasComments: Yes
[kudu-CR] KUDU-2033 (part 1). Add write and stop/start in a loop to TestLeaderFailover.
Edward Fancher has uploaded a new patch set (#2). Change subject: KUDU-2033 (part 1). Add write and stop/start in a loop to TestLeaderFailover. .. KUDU-2033 (part 1). Add write and stop/start in a loop to TestLeaderFailover. TestLeaderFailover is made more robust by adding a write loop in which the tablet leader is killed and restarted. The main idea is to check that the writes complete and can be read after the leader is killed. Done in a loop so we can be sure that we stay stable through multiple stops and restarts of the leader tablet. Change-Id: Ie93a15084a4c4c0748dc74c005b8313f443a5ba9 --- M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestLeaderFailover.java 2 files changed, 29 insertions(+), 12 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/7456/2 -- To view, visit http://gerrit.cloudera.org:8080/7456 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ie93a15084a4c4c0748dc74c005b8313f443a5ba9 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Edward Fancher Gerrit-Reviewer: Kudu Jenkins