You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Anonymous Coward (Code Review)" <ge...@cloudera.org> on 2017/08/21 04:15:22 UTC
[kudu-CR] (WIP) KUDU-2095 - Add `keepAlive` method to Java API
tony@phdata.io has uploaded a new change for review.
http://gerrit.cloudera.org:8080/7749
Change subject: (WIP) KUDU-2095 - Add `keepAlive` method to Java API
......................................................................
(WIP) KUDU-2095 - Add `keepAlive` method to Java API
Call `keepAlive` for cases when scanner ttl is too short
This needs a test but I haven't found an equivalent to the TEST_F
function in the C++ and the default timeout of the scanner needs to be
lowered for the unit test to run in a reasonable amount of time.
Error handling and test cases also need to be improved but I wanted to
get this out there to get feedback before I went any further.
Change-Id: Iba647e7b5ed4ee8d223d387b6656d3fb02c41713
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java
4 files changed, 126 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/49/7749/1
--
To view, visit http://gerrit.cloudera.org:8080/7749
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iba647e7b5ed4ee8d223d387b6656d3fb02c41713
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: tony@phdata.io
[kudu-CR] KUDU-2095 - Add `keepAlive` method to Java API
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7749
to look at the new patch set (#5).
Change subject: KUDU-2095 - Add `keepAlive` method to Java API
......................................................................
KUDU-2095 - Add `keepAlive` method to Java API
- Adds keepAlive methods to sync and async clients
- Methods return a (Deferred) Status object
Where possible the behavior mimics the C++ client.
Publicly accessible methods are available in AsyncKuduScanner and
KuduScanner.
A call to KeepAlive will return a Status object which will be OK if
- The scanner has been initialized
- The scanner is active and has rows left to return
- The scanner is between tablets
It will return a non-ok status when
- The scanner has not yet been initialized
- The scanner has no more rows
Change-Id: Iba647e7b5ed4ee8d223d387b6656d3fb02c41713
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanner.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java
4 files changed, 203 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/49/7749/5
--
To view, visit http://gerrit.cloudera.org:8080/7749
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iba647e7b5ed4ee8d223d387b6656d3fb02c41713
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: tony@phdata.io
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2095 - Add scanner `keepAlive` RPC call to Java API
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/7749 )
Change subject: KUDU-2095 - Add scanner `keepAlive` RPC call to Java API
......................................................................
Patch Set 9:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/7749/9/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java:
http://gerrit.cloudera.org:8080/#/c/7749/9/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@1006
PS9, Line 1006:
nit: double space
http://gerrit.cloudera.org:8080/#/c/7749/9/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@1007
PS9, Line 1007: defaultAdminOperationTimeoutMs
this seems like an odd choice of timeout. I think this defaults to be pretty long, maybe a short one is better?
http://gerrit.cloudera.org:8080/#/c/7749/9/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@1008
PS9, Line 1008: final ReplicaSelection replicaSelection = scanner.getReplicaSelection();
: final ServerInfo info =
: tablet.getReplicaSelectedServerInfo(replicaSelection);
is this guaranteed to be the same replica that the scanner is already open on? it doesn't seem so, though I'm not super familiar with this code. Given that replica selection might be somehow randomized this might get fired to some different server?
http://gerrit.cloudera.org:8080/#/c/7749/9/java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanner.java
File java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanner.java:
http://gerrit.cloudera.org:8080/#/c/7749/9/java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanner.java@75
PS9, Line 75: SetFaultTolerant
this is the C++ API name
http://gerrit.cloudera.org:8080/#/c/7749/9/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java
File java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java:
http://gerrit.cloudera.org:8080/#/c/7749/9/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java@203
PS9, Line 203: 50% of the scanner ttl.
I dont see anywhere setting a short TTL on this test, am I missing something?
--
To view, visit http://gerrit.cloudera.org:8080/7749
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iba647e7b5ed4ee8d223d387b6656d3fb02c41713
Gerrit-Change-Number: 7749
Gerrit-PatchSet: 9
Gerrit-Owner: Tony Foerster <an...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Tony Foerster <an...@gmail.com>
Gerrit-Comment-Date: Tue, 31 Jul 2018 03:00:54 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2095 - Add scanner `keepAlive` RPC call to Java API
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
tony@phdata.io has posted comments on this change.
Change subject: KUDU-2095 - Add scanner `keepAlive` RPC call to Java API
......................................................................
Patch Set 5:
(3 comments)
Notice I removed the public method from AsyncKuduClient, keepAlive can be called from the scanner only, not from the client with the scanner as an arg
http://gerrit.cloudera.org:8080/#/c/7749/3/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java:
Line 843: final KuduRpc<Status> keepAliveRequest = scanner.getKeepAliveRequest();
> this seems relatively suspicious. I see you copied it from 'close' above, b
Right the replica selected was always the master and it failed when I set the scanner ReplicaSelection.CLOSEST_REPLICA.
I then tried to use the scanner's ReplicaSelection and keepAlive works:
```
final ReplicaSelection replicaSelection = scanner.getReplicaSelection();
final ServerInfo info =
tablet.getReplicaSelectedServerInfo(replicaSelection);
```
but CLOSEST_REPLICAs documentation says it could choose a random replica. I don't know if this is actually correct or if it just happens to choose the same replica in my test.
http://gerrit.cloudera.org:8080/#/c/7749/2/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java:
Line 45: import org.apache.yetus.audience.InterfaceAudience;
Will remove
http://gerrit.cloudera.org:8080/#/c/7749/5/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java
File java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java:
Line 60: @BeforeClass
This runs ok and is like
--
To view, visit http://gerrit.cloudera.org:8080/7749
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iba647e7b5ed4ee8d223d387b6656d3fb02c41713
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: tony@phdata.io
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: tony@phdata.io
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2095 - Add scanner `keepAlive` RPC call to Java API
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7749
to look at the new patch set (#7).
Change subject: KUDU-2095 - Add scanner `keepAlive` RPC call to Java API
......................................................................
KUDU-2095 - Add scanner `keepAlive` RPC call to Java API
- Adds keepAlive methods to sync and async clients
- Methods return a (Deferred) Status object
Where possible the behavior mimics the C++ client.
Publicly accessible methods are available in AsyncKuduScanner and
KuduScanner.
A call to KeepAlive will return a Status object which will be OK if
- The scanner has been initialized
- The scanner is active and has rows left to return
- The scanner is between tablets
It will return a non-ok status when
- The scanner has not yet been initialized
- The scanner has no more rows
Change-Id: Iba647e7b5ed4ee8d223d387b6656d3fb02c41713
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanner.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java
4 files changed, 203 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/49/7749/7
--
To view, visit http://gerrit.cloudera.org:8080/7749
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iba647e7b5ed4ee8d223d387b6656d3fb02c41713
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: tony@phdata.io
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: tony@phdata.io
[kudu-CR] KUDU-2095 - Add `keepAlive` method to Java API
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7749
to look at the new patch set (#3).
Change subject: KUDU-2095 - Add `keepAlive` method to Java API
......................................................................
KUDU-2095 - Add `keepAlive` method to Java API
- Adds keepAlive methods to sync and async clients
- Methods return a (Deferred) Status object
Where possible the behavior mimics the C++ client. The C++ client has extra
properties that keep track of the last response, however adding this to the Java
client looked invasive and I did not implement it.
I added one test in TestKuduClient that also mirrors the C++. The test
runs in a KuduMiniCluster so ttl could be lowered. This means two
MiniClusters are running at the same time but it seems to cause no
problems and is cleaner than making a new test class. Even though the
TTL is low the time it takes for the scanner to be garbage collected is
long and the test takes ~10 seconds. The C++ client code comments that
the garbage collection of scanners runs each 50ms which is a lot faster
than it is running here but was unable to find a flag to lower it other
than scanner_ttl.
Change-Id: Iba647e7b5ed4ee8d223d387b6656d3fb02c41713
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanner.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java
5 files changed, 156 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/49/7749/3
--
To view, visit http://gerrit.cloudera.org:8080/7749
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iba647e7b5ed4ee8d223d387b6656d3fb02c41713
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: tony@phdata.io
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-2095 - Add scanner `keepAlive` RPC call to Java API
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
tony@phdata.io has posted comments on this change.
Change subject: KUDU-2095 - Add scanner `keepAlive` RPC call to Java API
......................................................................
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7749/7/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java:
Line 839: if (tablet == null) {
> I still have some work to do here I think
Maybe not, I think the behavior is correct. The other client has
```
// If there is no scanner to keep alive, we still return Status::OK().
if (!last_response_.IsInitialized() || !last_response_.has_more_results() ||
!next_req_.has_scanner_id()) {
return Status::OK();
}
```
And I think I cover all of those cases
--
To view, visit http://gerrit.cloudera.org:8080/7749
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iba647e7b5ed4ee8d223d387b6656d3fb02c41713
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: tony@phdata.io
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: tony@phdata.io
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2095 - Add scanner `keepAlive` RPC call to Java API
Posted by "Tony Foerster (Code Review)" <ge...@cloudera.org>.
Tony Foerster has abandoned this change. ( http://gerrit.cloudera.org:8080/7749 )
Change subject: KUDU-2095 - Add scanner `keepAlive` RPC call to Java API
......................................................................
Abandoned
Keepalive shouldn't rely on getting the same replica from the cached RemoteTablet.
--
To view, visit http://gerrit.cloudera.org:8080/7749
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: Iba647e7b5ed4ee8d223d387b6656d3fb02c41713
Gerrit-Change-Number: 7749
Gerrit-PatchSet: 9
Gerrit-Owner: Tony Foerster <an...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Tony Foerster <an...@gmail.com>
[kudu-CR] KUDU-2095 - Add `keepAlive` method to Java API
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-2095 - Add `keepAlive` method to Java API
......................................................................
Patch Set 3:
(14 comments)
http://gerrit.cloudera.org:8080/#/c/7749/3//COMMIT_MSG
Commit Message:
PS3, Line 13: properties that keep track of the last response, however adding this to the Java
: client looked invasive and I did not implement it.
not sure if this is a lack of functionality or just due to different implementation style?
PS3, Line 18: MiniClusters are running at the same time
Check out how TestAuthnTokenReacquire sets flags, for example, to avoid this
Line 23: than it is running here but was unable to find a flag to lower it other
I think you're looking for 'scanner_gc_check_interval_us'
http://gerrit.cloudera.org:8080/#/c/7749/3/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java:
Line 843: final ServerInfo info = tablet.getReplicaSelectedServerInfo(keepAliveRequest.getReplicaSelection());
this seems relatively suspicious. I see you copied it from 'close' above, but are we guaranteed that this returns the same ServerInfo as the server which is processing our currently-open scanner?
http://gerrit.cloudera.org:8080/#/c/7749/3/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java:
Line 45: import com.sun.tools.doclets.formats.html.SourceToHTMLConverter;
hrmm? this is causing a build failure
Line 645: * Keep the current remote scanner alive.
can we provide more docs on how this is meant to be used?
Line 652: final Deferred<Status> d =
nit: no wrap, can just combine with the 'return' below
PS3, Line 672: getKeepAliveRequest
nit: change to multiple lines. Add a javadoc.
Line 761: final Tserver.ScannerKeepAliveRequestPB.Builder builder = Tserver.ScannerKeepAliveRequestPB.newBuilder();
wrap
Line 781: String tsUUID) throws KuduException {
nit: alignment
Line 783: Tserver.ScannerKeepAliveResponsePB.Builder builder = Tserver.ScannerKeepAliveResponsePB.newBuilder();
nit: wrap
Line 785: Tserver.ScannerKeepAliveResponsePB resp = builder.build();
nit: extra space
Line 788: return new Pair<Status, Object>(Status.OK(), error);
nit: this whole function is indented one too much
http://gerrit.cloudera.org:8080/#/c/7749/3/java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanner.java
File java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanner.java:
PS3, Line 62: * Keep the current scanner alive on the tablet server.
: * @return Status of the RPC request
: * @throws KuduException
needs more clear docs
--
To view, visit http://gerrit.cloudera.org:8080/7749
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iba647e7b5ed4ee8d223d387b6656d3fb02c41713
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: tony@phdata.io
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2095 - Add scanner `keepAlive` RPC call to Java API
Posted by "Tony Foerster (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7749
to look at the new patch set (#9).
Change subject: KUDU-2095 - Add scanner `keepAlive` RPC call to Java API
......................................................................
KUDU-2095 - Add scanner `keepAlive` RPC call to Java API
- Adds keepAlive methods to sync and async clients
- Methods return a (Deferred) Status object
Where possible the behavior mimics the C++ client.
Publicly accessible methods are available in AsyncKuduScanner and
KuduScanner.
A call to KeepAlive will return a Status object which will be OK if
- The scanner has been initialized
- The scanner is active and has rows left to return
- The scanner is between tablets
It will return a non-ok status when
- The scanner has not yet been initialized
- The scanner has no more rows
Change-Id: Iba647e7b5ed4ee8d223d387b6656d3fb02c41713
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanner.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java
4 files changed, 190 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/49/7749/9
--
To view, visit http://gerrit.cloudera.org:8080/7749
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iba647e7b5ed4ee8d223d387b6656d3fb02c41713
Gerrit-Change-Number: 7749
Gerrit-PatchSet: 9
Gerrit-Owner: Tony Foerster <an...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Tony Foerster <an...@gmail.com>
[kudu-CR] (WIP) KUDU-2095 - Add `keepAlive` method to Java API
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7749
to look at the new patch set (#2).
Change subject: (WIP) KUDU-2095 - Add `keepAlive` method to Java API
......................................................................
(WIP) KUDU-2095 - Add `keepAlive` method to Java API
Call `keepAlive` for cases when scanner ttl is too short
This needs a test but I haven't found an equivalent to the TEST_F
function in the C++ and the default timeout of the scanner needs to be
lowered for the unit test to run in a reasonable amount of time.
Error handling and test cases also need to be improved but I wanted to
get this out there to get feedback before I went any further.
Change-Id: Iba647e7b5ed4ee8d223d387b6656d3fb02c41713
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java
4 files changed, 107 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/49/7749/2
--
To view, visit http://gerrit.cloudera.org:8080/7749
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iba647e7b5ed4ee8d223d387b6656d3fb02c41713
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: tony@phdata.io
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-2095 - Add `keepAlive` method to Java API
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7749
to look at the new patch set (#4).
Change subject: KUDU-2095 - Add `keepAlive` method to Java API
......................................................................
KUDU-2095 - Add `keepAlive` method to Java API
- Adds keepAlive methods to sync and async clients
- Methods return a (Deferred) Status object
Where possible the behavior mimics the C++ client.
Publicly accessible methods are available in AsyncKuduScanner and
KuduScanner.
A call to KeepAlive will return a Status object which will be OK if
- The scanner has been initialized
- The scanner is active and has rows left to return
- The scanner is between tablets
It will return a non-ok status when
- The scanner has not yet been initialized
- The scanner has no more rows
Change-Id: Iba647e7b5ed4ee8d223d387b6656d3fb02c41713
---
A java/kudu-client/kudu_style.xml
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanner.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java
5 files changed, 457 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/49/7749/4
--
To view, visit http://gerrit.cloudera.org:8080/7749
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iba647e7b5ed4ee8d223d387b6656d3fb02c41713
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: tony@phdata.io
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2095 - Add scanner `keepAlive` RPC call to Java API
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
tony@phdata.io has posted comments on this change.
Change subject: KUDU-2095 - Add scanner `keepAlive` RPC call to Java API
......................................................................
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7749/7/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java:
Line 839: if (tablet == null) {
I still have some work to do here I think
--
To view, visit http://gerrit.cloudera.org:8080/7749
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iba647e7b5ed4ee8d223d387b6656d3fb02c41713
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: tony@phdata.io
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: tony@phdata.io
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2095 - Add scanner `keepAlive` RPC call to Java API
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
tony@phdata.io has posted comments on this change.
Change subject: KUDU-2095 - Add scanner `keepAlive` RPC call to Java API
......................................................................
Patch Set 6:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/7749/5/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java:
Line 845: final ReplicaSelection replicaSelection = scanner.getReplicaSelection();
I think this is better. A scanner would fail with ReplicaSelection.CLOSEST_REPLICA. Now that passes but since CLOSEST_REPLICA could return a random replica I don't know that this is exactly correct.
http://gerrit.cloudera.org:8080/#/c/7749/6/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java:
Line 843: final KuduRpc<Status> keepAliveRequest = scanner.getKeepAliveRequest();
I think this is better, but since ReplicaSelection.CLOSEST_REPLICA can return a random replica if they're equidistant I'm not sure how I'd know which exact replica has the scanner. It seems to rely on the order a java.util.Map iterates values in RemoteTablet.
http://gerrit.cloudera.org:8080/#/c/7749/6/java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanner.java
File java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanner.java:
Line 62: * Keep the current remote scanner alive.
I took the long docs from C++ for the public methods
http://gerrit.cloudera.org:8080/#/c/7749/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java
File java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java:
Line 60: @BeforeClass
Is this a good idea to change the timeouts for all tests? Doesn't seem to cause problems for me but I don't want to cause any flakiness
--
To view, visit http://gerrit.cloudera.org:8080/7749
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iba647e7b5ed4ee8d223d387b6656d3fb02c41713
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: tony@phdata.io
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: tony@phdata.io
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2095 - Add scanner `keepAlive` RPC call to Java API
Posted by "Tony Foerster (Code Review)" <ge...@cloudera.org>.
Tony Foerster has posted comments on this change. ( http://gerrit.cloudera.org:8080/7749 )
Change subject: KUDU-2095 - Add scanner `keepAlive` RPC call to Java API
......................................................................
Patch Set 9:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/7749/9/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java:
http://gerrit.cloudera.org:8080/#/c/7749/9/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@1006
PS9, Line 1006:
> nit: double space
Done
http://gerrit.cloudera.org:8080/#/c/7749/9/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@1007
PS9, Line 1007: defaultAdminOperationTimeoutMs
> this seems like an odd choice of timeout. I think this defaults to be prett
I'm not sure exactly what's reasonable here? I set it to a constant 10 seconds. I also considered setting it to some division of defaultAdminOperationTimeoutMs, that way it would be somewhat configurable (but also more complicated maybe than it needs to be).
http://gerrit.cloudera.org:8080/#/c/7749/9/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@1008
PS9, Line 1008: final ReplicaSelection replicaSelection = scanner.getReplicaSelection();
: final ServerInfo info =
: tablet.getReplicaSelectedServerInfo(replicaSelection);
> is this guaranteed to be the same replica that the scanner is already open
I think the server is always the same but this behavior has been marked as a bug in the meantime: https://github.com/cloudera/kudu/blob/master/java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java#L191
I opened a pull request for this here: https://gerrit.cloudera.org/#/c/11088/
I kept the replica returned non-random per RemoteTablet instance, this way I won't have to keep track of replicas within the scanner. I think this is ok because it's unlikely (let me know if this is a bad assumption) for any client to read from the same tablet concurrently.
http://gerrit.cloudera.org:8080/#/c/7749/9/java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanner.java
File java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanner.java:
http://gerrit.cloudera.org:8080/#/c/7749/9/java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanner.java@75
PS9, Line 75: SetFaultTolerant
> this is the C++ API name
I shamelessly stole this from the C++ docs obviously. I need to double check that the behavior with isFaultTolerant is the same.
http://gerrit.cloudera.org:8080/#/c/7749/9/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java
File java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java:
http://gerrit.cloudera.org:8080/#/c/7749/9/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java@203
PS9, Line 203: 50% of the scanner ttl.
> I dont see anywhere setting a short TTL on this test, am I missing somethin
Not missing anything I removed mistakenly. I had been doing this for the entire test like https://gerrit.cloudera.org/#/c/7749/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java
I also tried just spawning another minicluster https://gerrit.cloudera.org/#/c/7749/3/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java
I am thinking I will move this to a new test class.
--
To view, visit http://gerrit.cloudera.org:8080/7749
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iba647e7b5ed4ee8d223d387b6656d3fb02c41713
Gerrit-Change-Number: 7749
Gerrit-PatchSet: 9
Gerrit-Owner: Tony Foerster <an...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Tony Foerster <an...@gmail.com>
Gerrit-Comment-Date: Tue, 31 Jul 2018 15:59:57 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2095 - Add scanner `keepAlive` RPC call to Java API
Posted by "Tony Foerster (Code Review)" <ge...@cloudera.org>.
Tony Foerster has posted comments on this change. ( http://gerrit.cloudera.org:8080/7749 )
Change subject: KUDU-2095 - Add scanner `keepAlive` RPC call to Java API
......................................................................
Patch Set 9: Code-Review-1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7749/9/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java:
http://gerrit.cloudera.org:8080/#/c/7749/9/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@1008
PS9, Line 1008: final ReplicaSelection replicaSelection = scanner.getReplicaSelection();
: final ServerInfo info =
: tablet.getReplicaSelectedServerInfo(replicaSelection);
> I think the server is always the same but this behavior has been marked as
Doesn't sound like relying on getting the same replica is the way to go, I'll look into keeping the replica data with a scanner.
--
To view, visit http://gerrit.cloudera.org:8080/7749
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iba647e7b5ed4ee8d223d387b6656d3fb02c41713
Gerrit-Change-Number: 7749
Gerrit-PatchSet: 9
Gerrit-Owner: Tony Foerster <an...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Tony Foerster <an...@gmail.com>
Gerrit-Comment-Date: Tue, 31 Jul 2018 20:15:45 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2095 - Add scanner `keepAlive` RPC call to Java API
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7749
to look at the new patch set (#6).
Change subject: KUDU-2095 - Add scanner `keepAlive` RPC call to Java API
......................................................................
KUDU-2095 - Add scanner `keepAlive` RPC call to Java API
- Adds keepAlive methods to sync and async clients
- Methods return a (Deferred) Status object
Where possible the behavior mimics the C++ client.
Publicly accessible methods are available in AsyncKuduScanner and
KuduScanner.
A call to KeepAlive will return a Status object which will be OK if
- The scanner has been initialized
- The scanner is active and has rows left to return
- The scanner is between tablets
It will return a non-ok status when
- The scanner has not yet been initialized
- The scanner has no more rows
Change-Id: Iba647e7b5ed4ee8d223d387b6656d3fb02c41713
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanner.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java
4 files changed, 206 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/49/7749/6
--
To view, visit http://gerrit.cloudera.org:8080/7749
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iba647e7b5ed4ee8d223d387b6656d3fb02c41713
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: tony@phdata.io
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2095 - Add scanner `keepAlive` RPC call to Java API
Posted by "Tony Foerster (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7749
to look at the new patch set (#8).
Change subject: KUDU-2095 - Add scanner `keepAlive` RPC call to Java API
......................................................................
KUDU-2095 - Add scanner `keepAlive` RPC call to Java API
- Adds keepAlive methods to sync and async clients
- Methods return a (Deferred) Status object
Where possible the behavior mimics the C++ client.
Publicly accessible methods are available in AsyncKuduScanner and
KuduScanner.
A call to KeepAlive will return a Status object which will be OK if
- The scanner has been initialized
- The scanner is active and has rows left to return
- The scanner is between tablets
It will return a non-ok status when
- The scanner has not yet been initialized
- The scanner has no more rows
Change-Id: Iba647e7b5ed4ee8d223d387b6656d3fb02c41713
---
A java/kudu-client/out/test/resources/log4j.properties
A java/kudu-client/out/test/resources/test-key-and-cert.jks
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanner.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java
6 files changed, 212 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/49/7749/8
--
To view, visit http://gerrit.cloudera.org:8080/7749
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iba647e7b5ed4ee8d223d387b6656d3fb02c41713
Gerrit-Change-Number: 7749
Gerrit-PatchSet: 8
Gerrit-Owner: Tony Foerster <an...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Tony Foerster <an...@gmail.com>