You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Edward Fancher (Code Review)" <ge...@cloudera.org> on 2017/07/18 22:01:03 UTC
[kudu-CR] KUDU-2033. Add a 'torture' scenario to verify Java client's behavior during fail-over
Edward Fancher has uploaded a new change for review.
http://gerrit.cloudera.org:8080/7456
Change subject: KUDU-2033. Add a 'torture' scenario to verify Java client's behavior during fail-over
......................................................................
KUDU-2033. Add a 'torture' scenario to verify Java client's behavior
during fail-over
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/1
--
To view, visit http://gerrit.cloudera.org:8080/7456
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie93a15084a4c4c0748dc74c005b8313f443a5ba9
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Edward Fancher <ef...@cloudera.com>
[kudu-CR] KUDU-2033 (part 1). Add write and stop/start in a loop to TestLeaderFailover
Posted by "Edward Fancher (Code Review)" <ge...@cloudera.org>.
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 <ef...@cloudera.com>
Gerrit-Reviewer: Edward Fancher <ef...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2033 (part 1). Add write and stop/start in a loop to TestLeaderFailover.
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 <ef...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2033 (part 1). Add write and stop/start in a loop to TestLeaderFailover
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 <ef...@cloudera.com>
Gerrit-Reviewer: Edward Fancher <ef...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] KUDU-2033 (part 1). Add write and stop/start in a loop to TestLeaderFailover
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 <ef...@cloudera.com>
Gerrit-Reviewer: Edward Fancher <ef...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] KUDU-2033 (part 1). Add write and stop/start in a loop to TestLeaderFailover
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 * 1000000;
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 <ef...@cloudera.com>
Gerrit-Reviewer: Edward Fancher <ef...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2033 (part 1). Add write and stop/start in a loop to TestLeaderFailover
Posted by "Edward Fancher (Code Review)" <ge...@cloudera.org>.
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 <ef...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2033 (part 1). Add write and stop/start in a loop to TestLeaderFailover
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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: 30000
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: 30000
DEFAULT_SLEEP
PS5, Line 93: 30000
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 <ef...@cloudera.com>
Gerrit-Reviewer: Edward Fancher <ef...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2033 (part 1). Add write and stop/start in a loop to TestLeaderFailover
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 <mp...@apache.org>
Tested-by: Mike Percy <mp...@apache.org>
---
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 <ef...@cloudera.com>
Gerrit-Reviewer: Edward Fancher <ef...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2033 (part 1). Add write and stop/start in a loop to TestLeaderFailover
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 <ef...@cloudera.com>
Gerrit-Reviewer: Edward Fancher <ef...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] KUDU-2033 (part 1). Add write and stop/start in a loop to TestLeaderFailover
Posted by "Edward Fancher (Code Review)" <ge...@cloudera.org>.
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 <ef...@cloudera.com>
Gerrit-Reviewer: Edward Fancher <ef...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2033 (part 1). Add write and stop/start in a loop to TestLeaderFailover
Posted by "Edward Fancher (Code Review)" <ge...@cloudera.org>.
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 <ef...@cloudera.com>
Gerrit-Reviewer: Edward Fancher <ef...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2033 (part 1). Add write and stop/start in a loop to TestLeaderFailover
Posted by "Edward Fancher (Code Review)" <ge...@cloudera.org>.
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 <ef...@cloudera.com>
Gerrit-Reviewer: Edward Fancher <ef...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2033 (part 1). Add write and stop/start in a loop to TestLeaderFailover
Posted by "Edward Fancher (Code Review)" <ge...@cloudera.org>.
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 <ef...@cloudera.com>
Gerrit-Reviewer: Edward Fancher <ef...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2033 (part 1). Add write and stop/start in a loop to TestLeaderFailover
Posted by "Edward Fancher (Code Review)" <ge...@cloudera.org>.
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 <ef...@cloudera.com>
Gerrit-Reviewer: Edward Fancher <ef...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2033 (part 1). Add write and stop/start in a loop to TestLeaderFailover
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 <ef...@cloudera.com>
Gerrit-Reviewer: Edward Fancher <ef...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2033 (part 1). Add write and stop/start in a loop to TestLeaderFailover
Posted by "Edward Fancher (Code Review)" <ge...@cloudera.org>.
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 <ef...@cloudera.com>
Gerrit-Reviewer: Edward Fancher <ef...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2033 (part 1). Add write and stop/start in a loop to TestLeaderFailover
Posted by "Edward Fancher (Code Review)" <ge...@cloudera.org>.
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 <ef...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2033 (part 1). Add write and stop/start in a loop to TestLeaderFailover
Posted by "Edward Fancher (Code Review)" <ge...@cloudera.org>.
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 <ef...@cloudera.com>
Gerrit-Reviewer: Edward Fancher <ef...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2033 (part 1). Add write and stop/start in a loop to TestLeaderFailover
Posted by "Edward Fancher (Code Review)" <ge...@cloudera.org>.
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 * 1000000;
: 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 <ef...@cloudera.com>
Gerrit-Reviewer: Edward Fancher <ef...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2033 (part 1). Add write and stop/start in a loop to TestLeaderFailover.
Posted by "Edward Fancher (Code Review)" <ge...@cloudera.org>.
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 <ef...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-2033 (part 1). Add write and stop/start in a loop to TestLeaderFailover
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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 <ef...@cloudera.com>
Gerrit-Reviewer: Edward Fancher <ef...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes