You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@kudu.apache.org by "Mike Percy (Code Review)" <ge...@cloudera.org> on 2016/12/10 23:05:49 UTC

[kudu-CR] KUDU-1767. Add java test for client operation interleaving

Hello Jean-Daniel Cryans, Alexey Serbin,

I'd like you to do a code review. Please visit

http://gerrit.cloudera.org:8080/5465

to review the following change.

Change subject: KUDU-1767. Add java test for client operation interleaving
......................................................................

KUDU-1767. Add java test for client operation interleaving

The test attempts to encourage operation interleaving by concurrently
flushing pairs of operations that conflict.

This test currently fails and so is disabled.

The version of JUnit had to be updated in the POM to 4.12 in order to
get support for simpler syntax when using parameterized tests that
only have a single parameter. See
https://github.com/junit-team/junit4/wiki/Parameterized-tests#tests-with-single-parameter

Change-Id: Idd30aeed34548b01ba4ad9b08cfd5fae22f967e9
---
A java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduSessionInterleaving.java
M java/pom.xml
2 files changed, 149 insertions(+), 1 deletion(-)

git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/65/5465/1
--
To view, visit http://gerrit.cloudera.org:8080/5465
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Idd30aeed34548b01ba4ad9b08cfd5fae22f967e9
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>

[kudu-CR] KUDU-1767. Add java test for client operation interleaving

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.

Mike Percy has abandoned this change.

Change subject: KUDU-1767. Add java test for client operation interleaving
......................................................................


Abandoned

We decided that, at least for now, this will not be fixed. See KUDU-1767.

-- 
To view, visit http://gerrit.cloudera.org:8080/5465
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: abandon
Gerrit-Change-Id: Idd30aeed34548b01ba4ad9b08cfd5fae22f967e9
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>

[kudu-CR] KUDU-1767. Add java test for client operation interleaving

Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.

Jean-Daniel Cryans has posted comments on this change.

Change subject: KUDU-1767. Add java test for client operation interleaving
......................................................................

Patch Set 2:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/5465/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduSessionInterleaving.java
File java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduSessionInterleaving.java:

Line 19: import java.util.ArrayList;
Your imports are in the wrong order, use the kudu_style.xml file to see what's wrong.

Line 64: final String TABLE_NAME = "test-table";
Usually we try to use the test's name for the table name.
Also, can you extract all those final fields?

Line 65: final long TIMEOUT_MILLIS = 30000;
We have a DEFAULT_SLEEP that comes from BaseKuduTest that we use, but based on my syncClient comment below you shouldn't even need to sleep.

Line 74: AsyncKuduSession session = client.newSession();
I'd encourage you to use the syncClient, since you're not making use of any specific async feature in this test. It will also simplify your test.

Line 102: Thread.sleep(THROTTLE_SLEEP_MS);
FYI the way to handle this is to join on the Deferred provided with the exception, this way you just wait what you need to wait.

Line 115: for (Deferred<OperationResponse> d : applyDeferreds) {
Doing this pretty much voids your warranty in the async client with AUTO_FLUSH_SYNC, the client doesn't guarantee correct ordering between calls if you're not waiting for each operation to finish before sending the next one. For correct batching you'd want MANUAL_FLUSH or AUTO_FLUSH_BACKGROUND.

You might be hitting this issue when your test fails, instead of the server-side issue you're trying to trigger. Using the sync client avoids this problem.

Line 135:
To be clean, with AUTO_FLUSH_BACKGROUND you should also check the error collector.

--
To view, visit http://gerrit.cloudera.org:8080/5465
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Idd30aeed34548b01ba4ad9b08cfd5fae22f967e9
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1767. Add java test for client operation interleaving

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.

Hello Kudu Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/5465

to look at the new patch set (#2).

Change subject: KUDU-1767. Add java test for client operation interleaving
......................................................................

KUDU-1767. Add java test for client operation interleaving

The test attempts to encourage operation interleaving by concurrently
flushing pairs of operations that conflict.

This test currently fails and so is disabled.

git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/65/5465/2
--
To view, visit http://gerrit.cloudera.org:8080/5465
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idd30aeed34548b01ba4ad9b08cfd5fae22f967e9
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>

[kudu-CR] KUDU-1767. Add java test for client operation interleaving

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.

Mike Percy has posted comments on this change.

Change subject: KUDU-1767. Add java test for client operation interleaving
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5465/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduSessionInterleaving.java
File java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduSessionInterleaving.java:

PS1, Line 61: //@Ignore
Whoops, forgot to uncomment @Ignore. Well, there's the Jenkins proof that the test fails :)


-- 
To view, visit http://gerrit.cloudera.org:8080/5465
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Idd30aeed34548b01ba4ad9b08cfd5fae22f967e9
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-HasComments: Yes