[kudu-CR] java: prohibit use of a KuduTable from an unassociated KuduClient

2018-04-30 Thread Alexey Serbin (Code Review) 
Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7362 )

Change subject: java: prohibit use of a KuduTable from an unassociated 
KuduClient
..


Patch Set 6: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib4a977f36b9c7ba3758322a2216ce90208b5d014
Gerrit-Change-Number: 7362
Gerrit-PatchSet: 6
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Anonymous Coward #314
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Mon, 30 Apr 2018 19:41:06 +
Gerrit-HasComments: No

[kudu-CR] java: prohibit use of a KuduTable from an unassociated KuduClient

2018-04-30 Thread David Ribeiro Alves (Code Review) 
David Ribeiro Alves has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7362 )

Change subject: java: prohibit use of a KuduTable from an unassociated 
KuduClient
..


Patch Set 6: Code-Review+2

thanks


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib4a977f36b9c7ba3758322a2216ce90208b5d014
Gerrit-Change-Number: 7362
Gerrit-PatchSet: 6
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Anonymous Coward #314
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Mon, 30 Apr 2018 19:29:24 +
Gerrit-HasComments: No

[kudu-CR] java: prohibit use of a KuduTable from an unassociated KuduClient

2018-04-30 Thread Todd Lipcon (Code Review) 
Todd Lipcon has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7362 )

Change subject: java: prohibit use of a KuduTable from an unassociated 
KuduClient
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7362/5/java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java
File java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java:

http://gerrit.cloudera.org:8080/#/c/7362/5/java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java@241
PS5, Line 241: so we shouldn't unregister it.
> oh, I guess the TODO is more what I added elsewhere that it doesn't really
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib4a977f36b9c7ba3758322a2216ce90208b5d014
Gerrit-Change-Number: 7362
Gerrit-PatchSet: 4
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Anonymous Coward #314
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Mon, 30 Apr 2018 19:24:14 +
Gerrit-HasComments: Yes

[kudu-CR] java: prohibit use of a KuduTable from an unassociated KuduClient

2018-04-30 Thread Todd Lipcon (Code Review) 
Hello Alexey Serbin, David Ribeiro Alves, Jean-Daniel Cryans, Kudu Jenkins, 
Anonymous Coward #314, Grant Henke,

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

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

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

Change subject: java: prohibit use of a KuduTable from an unassociated 
KuduClient
..

java: prohibit use of a KuduTable from an unassociated KuduClient

This fixes a request-tracking issue with the following code
anti-pattern in which a KuduTable associated with one client is used to
create operations applied to another client's session:

  KuduClient client1 = KuduClientBuildernewClient();
  KuduTable t = client1.openTable(...);
  KuduClient client2 = KuduClientBuildernewClient();
  KuduSession s = client2.newSession();
  s.apply(t.newUpdate());

This would cause sequence numbers to be generated out of the session's
client's RequestTracker, but then marked complete in the operation's
client's RequestTracker. This could cause any number of issues:

- a cache "hit" on the server side might cause an operation to get an
  unrelated operation's response
- a cache "miss" on the server side might result in an operation
  incorrectly being marked as already-responded and garbage collected,
  causing it to fail.

This patch adds a Preconditions check for this issue and fixes some tests
where it was triggered.

Change-Id: Ib4a977f36b9c7ba3758322a2216ce90208b5d014
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RequestTracker.java
M java/kudu-client/src/test/java/org/apache/kudu/client/ITClientStress.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestRequestTracker.java
M 
java/kudu-client/src/test/java/org/apache/kudu/client/TestScannerMultiTablet.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestTimeouts.java
9 files changed, 104 insertions(+), 58 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/7362/6
--
To view, visit http://gerrit.cloudera.org:8080/7362
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib4a977f36b9c7ba3758322a2216ce90208b5d014
Gerrit-Change-Number: 7362
Gerrit-PatchSet: 6
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Anonymous Coward #314
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] java: prohibit use of a KuduTable from an unassociated KuduClient

2018-04-26 Thread Todd Lipcon (Code Review) 
Todd Lipcon has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7362 )

Change subject: java: prohibit use of a KuduTable from an unassociated 
KuduClient
..


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7362/5/java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java
File java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java:

http://gerrit.cloudera.org:8080/#/c/7362/5/java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java@241
PS5, Line 241: so we shouldn't unregister it.
> my suggestion referred to the fact that this doesn't seem a TODO just a gen
oh, I guess the TODO is more what I added elsewhere that it doesn't really make 
sense for Operation to be a subclass of KuduRpc, since it duplicates a lot of 
stuff with 'Batch'. I'll reword.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib4a977f36b9c7ba3758322a2216ce90208b5d014
Gerrit-Change-Number: 7362
Gerrit-PatchSet: 5
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Anonymous Coward #314
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Thu, 26 Apr 2018 22:31:30 +
Gerrit-HasComments: Yes

[kudu-CR] java: prohibit use of a KuduTable from an unassociated KuduClient

2018-04-26 Thread David Ribeiro Alves (Code Review) 
David Ribeiro Alves has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7362 )

Change subject: java: prohibit use of a KuduTable from an unassociated 
KuduClient
..


Patch Set 5:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/7362/5/java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java
File java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java:

http://gerrit.cloudera.org:8080/#/c/7362/5/java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java@241
PS5, Line 241: so we shouldn't unregister it.
> well, it will crash due to the assertion I just added. I thought the level
my suggestion referred to the fact that this doesn't seem a TODO just a general 
(albeit useful) statement. What's left TODO?


http://gerrit.cloudera.org:8080/#/c/7362/5/java/kudu-client/src/main/java/org/apache/kudu/client/RequestTracker.java
File java/kudu-client/src/main/java/org/apache/kudu/client/RequestTracker.java:

http://gerrit.cloudera.org:8080/#/c/7362/5/java/kudu-client/src/main/java/org/apache/kudu/client/RequestTracker.java@85
PS5, Line 85: assert(removed) : "Could not remove seqid " + sequenceId + " from 
request tracker";
> yea, our tests run with -ea (while I was working on this I caused test fail
k, thanks



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib4a977f36b9c7ba3758322a2216ce90208b5d014
Gerrit-Change-Number: 7362
Gerrit-PatchSet: 5
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Anonymous Coward #314
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Thu, 26 Apr 2018 22:30:12 +
Gerrit-HasComments: Yes

[kudu-CR] java: prohibit use of a KuduTable from an unassociated KuduClient

2018-04-26 Thread Todd Lipcon (Code Review) 
Todd Lipcon has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7362 )

Change subject: java: prohibit use of a KuduTable from an unassociated 
KuduClient
..


Patch Set 5:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/7362/5/java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java
File java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java:

http://gerrit.cloudera.org:8080/#/c/7362/5/java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java@241
PS5, Line 241: so we shouldn't unregister it.
> maybe mention the pitfalls of unregistering it? (i.e. why we care about thi
well, it will crash due to the assertion I just added. I thought the level of 
detail here was OK since it doesn't make sense to unregister something that 
wasn't registered


http://gerrit.cloudera.org:8080/#/c/7362/5/java/kudu-client/src/main/java/org/apache/kudu/client/RequestTracker.java
File java/kudu-client/src/main/java/org/apache/kudu/client/RequestTracker.java:

http://gerrit.cloudera.org:8080/#/c/7362/5/java/kudu-client/src/main/java/org/apache/kudu/client/RequestTracker.java@85
PS5, Line 85: assert(removed) : "Could not remove seqid " + sequenceId + " from 
request tracker";
> do we ever compile with -ea ? i forget
yea, our tests run with -ea (while I was working on this I caused test failures 
due to not handling the corner case of NO_SEQ_NO)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib4a977f36b9c7ba3758322a2216ce90208b5d014
Gerrit-Change-Number: 7362
Gerrit-PatchSet: 5
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Anonymous Coward #314
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Thu, 26 Apr 2018 22:17:47 +
Gerrit-HasComments: Yes

[kudu-CR] java: prohibit use of a KuduTable from an unassociated KuduClient

2018-04-26 Thread David Ribeiro Alves (Code Review) 
David Ribeiro Alves has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7362 )

Change subject: java: prohibit use of a KuduTable from an unassociated 
KuduClient
..


Patch Set 5:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/7362/5/java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java
File java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java:

http://gerrit.cloudera.org:8080/#/c/7362/5/java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java@241
PS5, Line 241: so we shouldn't unregister it.
maybe mention the pitfalls of unregistering it? (i.e. why we care about this) 
just extra contention or something more?


http://gerrit.cloudera.org:8080/#/c/7362/5/java/kudu-client/src/main/java/org/apache/kudu/client/RequestTracker.java
File java/kudu-client/src/main/java/org/apache/kudu/client/RequestTracker.java:

http://gerrit.cloudera.org:8080/#/c/7362/5/java/kudu-client/src/main/java/org/apache/kudu/client/RequestTracker.java@85
PS5, Line 85: assert(removed) : "Could not remove seqid " + sequenceId + " from 
request tracker";
do we ever compile with -ea ? i forget



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib4a977f36b9c7ba3758322a2216ce90208b5d014
Gerrit-Change-Number: 7362
Gerrit-PatchSet: 5
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Anonymous Coward #314
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Thu, 26 Apr 2018 20:02:14 +
Gerrit-HasComments: Yes

[kudu-CR] java: prohibit use of a KuduTable from an unassociated KuduClient

2018-04-25 Thread Todd Lipcon (Code Review) 
Hello Alexey Serbin, David Ribeiro Alves, Jean-Daniel Cryans, Kudu Jenkins, 
Anonymous Coward #314, Grant Henke,

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

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

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

Change subject: java: prohibit use of a KuduTable from an unassociated 
KuduClient
..

java: prohibit use of a KuduTable from an unassociated KuduClient

This fixes a request-tracking issue with the following code
anti-pattern in which a KuduTable associated with one client is used to
create operations applied to another client's session:

  KuduClient client1 = KuduClientBuildernewClient();
  KuduTable t = client1.openTable(...);
  KuduClient client2 = KuduClientBuildernewClient();
  KuduSession s = client2.newSession();
  s.apply(t.newUpdate());

This would cause sequence numbers to be generated out of the session's
client's RequestTracker, but then marked complete in the operation's
client's RequestTracker. This could cause any number of issues:

- a cache "hit" on the server side might cause an operation to get an
  unrelated operation's response
- a cache "miss" on the server side might result in an operation
  incorrectly being marked as already-responded and garbage collected,
  causing it to fail.

This patch adds a Preconditions check for this issue and fixes some tests
where it was triggered.

Change-Id: Ib4a977f36b9c7ba3758322a2216ce90208b5d014
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RequestTracker.java
M java/kudu-client/src/test/java/org/apache/kudu/client/ITClientStress.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestRequestTracker.java
M 
java/kudu-client/src/test/java/org/apache/kudu/client/TestScannerMultiTablet.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestTimeouts.java
9 files changed, 106 insertions(+), 58 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/7362/4
--
To view, visit http://gerrit.cloudera.org:8080/7362
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib4a977f36b9c7ba3758322a2216ce90208b5d014
Gerrit-Change-Number: 7362
Gerrit-PatchSet: 4
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Anonymous Coward #314
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] java: prohibit use of a KuduTable from an unassociated KuduClient

2018-04-25 Thread Todd Lipcon (Code Review) 
Todd Lipcon has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7362 )

Change subject: java: prohibit use of a KuduTable from an unassociated 
KuduClient
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7362/3/java/kudu-client/src/main/java/org/apache/kudu/client/RequestTracker.java
File java/kudu-client/src/main/java/org/apache/kudu/client/RequestTracker.java:

http://gerrit.cloudera.org:8080/#/c/7362/3/java/kudu-client/src/main/java/org/apache/kudu/client/RequestTracker.java@76
PS3, Line 76: This operation is idempotent.
> Nit: with this change this statement becomes a little funky if debug is tur
oops, yea



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib4a977f36b9c7ba3758322a2216ce90208b5d014
Gerrit-Change-Number: 7362
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Anonymous Coward #314
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 25 Apr 2018 19:28:54 +
Gerrit-HasComments: Yes

[kudu-CR] java: prohibit use of a KuduTable from an unassociated KuduClient

2018-04-24 Thread Alexey Serbin (Code Review) 
Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7362 )

Change subject: java: prohibit use of a KuduTable from an unassociated 
KuduClient
..


Patch Set 3: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7362/3/java/kudu-client/src/main/java/org/apache/kudu/client/RequestTracker.java
File java/kudu-client/src/main/java/org/apache/kudu/client/RequestTracker.java:

http://gerrit.cloudera.org:8080/#/c/7362/3/java/kudu-client/src/main/java/org/apache/kudu/client/RequestTracker.java@76
PS3, Line 76: This operation is idempotent.
Nit: with this change this statement becomes a little funky if debug is turned 
on.  Also, with this change the code says 'don't call this method twice'.  
Maybe, remove this statement to avoid confusion?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib4a977f36b9c7ba3758322a2216ce90208b5d014
Gerrit-Change-Number: 7362
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Anonymous Coward #314
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 25 Apr 2018 00:23:32 +
Gerrit-HasComments: Yes

[kudu-CR] java: prohibit use of a KuduTable from an unassociated KuduClient

2018-04-24 Thread Todd Lipcon (Code Review) 
Hello Alexey Serbin, David Ribeiro Alves, Jean-Daniel Cryans, Kudu Jenkins, 
Anonymous Coward #314,

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

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

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

Change subject: java: prohibit use of a KuduTable from an unassociated 
KuduClient
..

java: prohibit use of a KuduTable from an unassociated KuduClient

This fixes a request-tracking issue with the following code
anti-pattern in which a KuduTable associated with one client is used to
create operations applied to another client's session:

  KuduClient client1 = KuduClientBuildernewClient();
  KuduTable t = client1.openTable(...);
  KuduClient client2 = KuduClientBuildernewClient();
  KuduSession s = client2.newSession();
  s.apply(t.newUpdate());

This would cause sequence numbers to be generated out of the session's
client's RequestTracker, but then marked complete in the operation's
client's RequestTracker. This could cause any number of issues:

- a cache "hit" on the server side might cause an operation to get an
  unrelated operation's response
- a cache "miss" on the server side might result in an operation
  incorrectly being marked as already-responded and garbage collected,
  causing it to fail.

This patch adds a Preconditions check for this issue and fixes some tests
where it was triggered.

Change-Id: Ib4a977f36b9c7ba3758322a2216ce90208b5d014
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RequestTracker.java
M java/kudu-client/src/test/java/org/apache/kudu/client/ITClientStress.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestRequestTracker.java
M 
java/kudu-client/src/test/java/org/apache/kudu/client/TestScannerMultiTablet.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestTimeouts.java
9 files changed, 103 insertions(+), 57 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/7362/3
--
To view, visit http://gerrit.cloudera.org:8080/7362
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib4a977f36b9c7ba3758322a2216ce90208b5d014
Gerrit-Change-Number: 7362
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Anonymous Coward #314
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] java: prohibit use of a KuduTable from an unassociated KuduClient

2017-08-18 Thread Todd Lipcon (Code Review) 
Todd Lipcon has posted comments on this change.

Change subject: java: prohibit use of a KuduTable from an unassociated 
KuduClient
..


Patch Set 2:

sure, will do.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib4a977f36b9c7ba3758322a2216ce90208b5d014
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] java: prohibit use of a KuduTable from an unassociated KuduClient

2017-08-18 Thread Jean-Daniel Cryans (Code Review) 
Jean-Daniel Cryans has posted comments on this change.

Change subject: java: prohibit use of a KuduTable from an unassociated 
KuduClient
..


Patch Set 2:

Todd, is it possible to rewrite this patch without the change to request 
tracking?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib4a977f36b9c7ba3758322a2216ce90208b5d014
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] java: prohibit use of a KuduTable from an unassociated KuduClient

2017-07-24 Thread Todd Lipcon (Code Review) 
Todd Lipcon has posted comments on this change.

Change subject: java: prohibit use of a KuduTable from an unassociated 
KuduClient
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7362/2/java/kudu-client/src/main/java/org/apache/kudu/client/RequestTracker.java
File java/kudu-client/src/main/java/org/apache/kudu/client/RequestTracker.java:

Line 72: // TODO(todd): this is weird, why are we calling completed on a -1 
seqno.
> I suspect that might happen if the rpc failed before we try to send it (whe
yea I did see it being triggered but didn't have time to figure out the exact 
"flow". David might be on the right track. I'll continue to investigate this.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib4a977f36b9c7ba3758322a2216ce90208b5d014
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] java: prohibit use of a KuduTable from an unassociated KuduClient

2017-07-24 Thread David Ribeiro Alves (Code Review) 
David Ribeiro Alves has posted comments on this change.

Change subject: java: prohibit use of a KuduTable from an unassociated 
KuduClient
..


Patch Set 2:

(1 comment)

this looks like a possible source of the problem. not privy enough about the 
spark client to assert whether that is the likely cause in that case.

http://gerrit.cloudera.org:8080/#/c/7362/2/java/kudu-client/src/main/java/org/apache/kudu/client/RequestTracker.java
File java/kudu-client/src/main/java/org/apache/kudu/client/RequestTracker.java:

Line 72: // TODO(todd): this is weird, why are we calling completed on a -1 
seqno.
> Have you seen -1 seqno being passed here?
I suspect that might happen if the rpc failed before we try to send it (when we 
set the seqno) somehow. the callback gets called without a seqno having been 
set (and isRequestTracked only depends on the type)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib4a977f36b9c7ba3758322a2216ce90208b5d014
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] java: prohibit use of a KuduTable from an unassociated KuduClient

2017-07-10 Thread Todd Lipcon (Code Review) 
Hello Kudu Jenkins,

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

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

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

Change subject: java: prohibit use of a KuduTable from an unassociated 
KuduClient
..

java: prohibit use of a KuduTable from an unassociated KuduClient

This fixes a request-tracking issue with the following code
anti-pattern in which a KuduTable associated with one client is used to
create operations applied to another client's session:

  KuduClient client1 = KuduClientBuildernewClient();
  KuduTable t = client1.openTable(...);
  KuduClient client2 = KuduClientBuildernewClient();
  KuduSession s = client2.newSession();
  s.apply(t.newUpdate());

This would cause sequence numbers to be generated out of the session's
client's RequestTracker, but then marked complete in the operation's
client's RequestTracker (or perhaps vice versa, not entirely sure).
Either way, this was bad, and seems quite likely to be the root cause of
KUDU-2053.

This patch adds a Preconditions check for this issue and fixes some tests
where it was triggered.

Change-Id: Ib4a977f36b9c7ba3758322a2216ce90208b5d014
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RequestTracker.java
M java/kudu-client/src/test/java/org/apache/kudu/client/ITClientStress.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestRequestTracker.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestTimeouts.java
5 files changed, 37 insertions(+), 40 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib4a977f36b9c7ba3758322a2216ce90208b5d014
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins