[kudu-CR] [java-client] use tablet cache for locateTablet calls
Jean-Daniel Cryans has posted comments on this change. Change subject: [java-client] use tablet cache for locateTablet calls .. Patch Set 5: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/3386 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I78b5d400778547a9ee090111663677901dbadd98 Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR] [java-client] use tablet cache for locateTablet calls
Jean-Daniel Cryans has submitted this change and it was merged. Change subject: [java-client] use tablet cache for locateTablet calls .. [java-client] use tablet cache for locateTablet calls This simplifies and improves the performance of AsyncKuduClient::locateTable by using the client's existing tablet cache. This change is motivated by an upcoming fix to the locate table logic, which will be easier without the code duplication. The public KuduTable::locateTable methods have been deprecated since they rely on callers having knowledge of the private internals of Kudu's partitioning implementation. Additionally, the methods have been changed to take an exclusive end partition key instead of an inclusive end partition key. Whether the key was inclusive or exclusive was previously undocumented, and the only known caller (the internal ScanToken implementation) was using it as exclusive. Users of these methods are encouraged to use the ScanToken API instead, since it's easier to use and does not leak internal Kudu implementation details. Change-Id: I78b5d400778547a9ee090111663677901dbadd98 Reviewed-on: http://gerrit.cloudera.org:8080/3386 Reviewed-by: Adar Dembo Tested-by: Kudu Jenkins Reviewed-by: Jean-Daniel Cryans --- M docs/release_notes.adoc M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java M java/kudu-client/src/main/java/org/kududb/client/KuduTable.java M java/kudu-client/src/main/java/org/kududb/client/LocatedTablet.java M java/kudu-client/src/test/java/org/kududb/client/TestKuduTable.java 5 files changed, 134 insertions(+), 93 deletions(-) Approvals: Jean-Daniel Cryans: Looks good to me, approved Adar Dembo: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/3386 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I78b5d400778547a9ee090111663677901dbadd98 Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [java-client] use tablet cache for locateTablet calls
Kudu Jenkins has posted comments on this change. Change subject: [java-client] use tablet cache for locateTablet calls .. Patch Set 5: -Verified Build Started http://104.196.14.100/job/kudu-gerrit/1862/ -- To view, visit http://gerrit.cloudera.org:8080/3386 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I78b5d400778547a9ee090111663677901dbadd98 Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR] [java-client] use tablet cache for locateTablet calls
Adar Dembo has posted comments on this change. Change subject: [java-client] use tablet cache for locateTablet calls .. Patch Set 5: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/3386 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I78b5d400778547a9ee090111663677901dbadd98 Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR] [java-client] use tablet cache for locateTablet calls
Kudu Jenkins has posted comments on this change. Change subject: [java-client] use tablet cache for locateTablet calls .. Patch Set 5: Build Started http://104.196.14.100/job/kudu-gerrit/1860/ -- To view, visit http://gerrit.cloudera.org:8080/3386 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I78b5d400778547a9ee090111663677901dbadd98 Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR] [java-client] use tablet cache for locateTablet calls
Hello Jean-Daniel Cryans, Adar Dembo, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/3386 to look at the new patch set (#5). Change subject: [java-client] use tablet cache for locateTablet calls .. [java-client] use tablet cache for locateTablet calls This simplifies and improves the performance of AsyncKuduClient::locateTable by using the client's existing tablet cache. This change is motivated by an upcoming fix to the locate table logic, which will be easier without the code duplication. The public KuduTable::locateTable methods have been deprecated since they rely on callers having knowledge of the private internals of Kudu's partitioning implementation. Additionally, the methods have been changed to take an exclusive end partition key instead of an inclusive end partition key. Whether the key was inclusive or exclusive was previously undocumented, and the only known caller (the internal ScanToken implementation) was using it as exclusive. Users of these methods are encouraged to use the ScanToken API instead, since it's easier to use and does not leak internal Kudu implementation details. Change-Id: I78b5d400778547a9ee090111663677901dbadd98 --- M docs/release_notes.adoc M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java M java/kudu-client/src/main/java/org/kududb/client/KuduTable.java M java/kudu-client/src/main/java/org/kududb/client/LocatedTablet.java M java/kudu-client/src/test/java/org/kududb/client/TestKuduTable.java 5 files changed, 134 insertions(+), 93 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/86/3386/5 -- To view, visit http://gerrit.cloudera.org:8080/3386 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I78b5d400778547a9ee090111663677901dbadd98 Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [java-client] use tablet cache for locateTablet calls
Kudu Jenkins has posted comments on this change. Change subject: [java-client] use tablet cache for locateTablet calls .. Patch Set 4: Build Started http://104.196.14.100/job/kudu-gerrit/1855/ -- To view, visit http://gerrit.cloudera.org:8080/3386 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I78b5d400778547a9ee090111663677901dbadd98 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR] [java-client] use tablet cache for locateTablet calls
Adar Dembo has posted comments on this change. Change subject: [java-client] use tablet cache for locateTablet calls .. Patch Set 4: Code-Review+2 (1 comment) Feel free to punt on the nit if you want to merge right now. Also, run the deprecation by JD. http://gerrit.cloudera.org:8080/#/c/3386/4/java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java File java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java: Line 83: import java.util.concurrent.*; Nit: personally this feels a little strange outside of tests. If there aren't a lot of imports, would you mind expanding this? -- To view, visit http://gerrit.cloudera.org:8080/3386 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I78b5d400778547a9ee090111663677901dbadd98 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
[kudu-CR] [java-client] use tablet cache for locateTablet calls
Kudu Jenkins has posted comments on this change. Change subject: [java-client] use tablet cache for locateTablet calls .. Patch Set 3: -Verified Build Started http://104.196.14.100/job/kudu-gerrit/1841/ -- To view, visit http://gerrit.cloudera.org:8080/3386 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I78b5d400778547a9ee090111663677901dbadd98 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR] [java-client] use tablet cache for locateTablet calls
Kudu Jenkins has posted comments on this change. Change subject: [java-client] use tablet cache for locateTablet calls .. Patch Set 4: Build Started http://104.196.14.100/job/kudu-gerrit/1838/ -- To view, visit http://gerrit.cloudera.org:8080/3386 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I78b5d400778547a9ee090111663677901dbadd98 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR] [java-client] use tablet cache for locateTablet calls
Hello Jean-Daniel Cryans, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/3386 to look at the new patch set (#4). Change subject: [java-client] use tablet cache for locateTablet calls .. [java-client] use tablet cache for locateTablet calls This simplifies and improves the performance of AsyncKuduClient::locateTable by using the client's existing tablet cache. This change is motivated by an upcoming fix to the locate table logic, which will be easier without the code duplication. The public KuduTable::locateTable methods have been deprecated since they rely on callers having knowledge of the private internals of Kudu's partitioning implementation. Additionally, the methods have been changed to take an exclusive end partition key instead of an inclusive end partition key. Whether the key was inclusive or exclusive was previously undocumented, and the only known caller (the internal ScanToken implementation) was using it as exclusive. Users of these methods are encouraged to use the ScanToken API instead, since it's easier to use and does not leak internal Kudu implementation details. Change-Id: I78b5d400778547a9ee090111663677901dbadd98 --- M docs/release_notes.adoc M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java M java/kudu-client/src/main/java/org/kududb/client/KuduTable.java M java/kudu-client/src/main/java/org/kududb/client/LocatedTablet.java M java/kudu-client/src/test/java/org/kududb/client/TestKuduTable.java 5 files changed, 128 insertions(+), 92 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/86/3386/4 -- To view, visit http://gerrit.cloudera.org:8080/3386 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I78b5d400778547a9ee090111663677901dbadd98 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [java-client] use tablet cache for locateTablet calls
Dan Burkert has posted comments on this change.
Change subject: [java-client] use tablet cache for locateTablet calls
..
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/3386/2//COMMIT_MSG
Commit Message:
PS2, Line 9: This simplifies and improves the performance of
AsyncKuduClient::locateTable by
: using the client's existing tablet cache.
> Nit: there's not enough context in this patch to know why you're making thi
Done
http://gerrit.cloudera.org:8080/#/c/3386/2/java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java
File java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java:
Line 1230: private final class MasterLookupCB implements Callback {
> Nit: too long now? Unclear.
Done
Line 2047: List getReplicas() {
> Quickly looking through the code, looks like we don't do it, and I'm not su
Immutability should be the default IMO.
--
To view, visit http://gerrit.cloudera.org:8080/3386
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I78b5d400778547a9ee090111663677901dbadd98
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] [java-client] use tablet cache for locateTablet calls
Kudu Jenkins has posted comments on this change. Change subject: [java-client] use tablet cache for locateTablet calls .. Patch Set 3: Build Started http://104.196.14.100/job/kudu-gerrit/1837/ -- To view, visit http://gerrit.cloudera.org:8080/3386 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I78b5d400778547a9ee090111663677901dbadd98 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR] [java-client] use tablet cache for locateTablet calls
Hello Jean-Daniel Cryans, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/3386 to look at the new patch set (#3). Change subject: [java-client] use tablet cache for locateTablet calls .. [java-client] use tablet cache for locateTablet calls This simplifies and improves the performance of AsyncKuduClient::locateTable by using the client's existing tablet cache. The semantics of locateTable are changed slightly- the end key is now treated as an exclusive key instead of inclusive. This change is motivated by an upcoming fix to the locate table logic, which will be easier without the code duplication. The public KuduTable::locateTable methods have been deprecated since they rely on callers having knowledge of the private internals of Kudu's partitioning implementation. Additionally, the methods have been changed to take an exclusive end partition key instead of an inclusive end partition key. Whether the key was inclusive or exclusive was previously undocumented, and the only known caller (the internal ScanToken implementation) was using it as exclusive. Users of these methods are encouraged to use the ScanToken API instead, since it's easier to use and does not leak internal Kudu implementation details. Change-Id: I78b5d400778547a9ee090111663677901dbadd98 --- M docs/release_notes.adoc M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java M java/kudu-client/src/main/java/org/kududb/client/KuduTable.java M java/kudu-client/src/main/java/org/kududb/client/LocatedTablet.java M java/kudu-client/src/test/java/org/kududb/client/TestKuduTable.java 5 files changed, 131 insertions(+), 92 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/86/3386/3 -- To view, visit http://gerrit.cloudera.org:8080/3386 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I78b5d400778547a9ee090111663677901dbadd98 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [java-client] use tablet cache for locateTablet calls
Dan Burkert has posted comments on this change.
Change subject: [java-client] use tablet cache for locateTablet calls
..
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/3386/2/java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java
File java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java:
Line 1876: private final List replicas = new
ArrayList<>();
> Makes sense to a degree, though bear in mind we're already doing DNS resolu
COWAL was my first thought here, but from a look through the API I couldn't
figure out a way to atomically swap the entire contents of the list. Now that
I think about it, though, what we want is a AtomicReference.
I'll make the change.
http://gerrit.cloudera.org:8080/#/c/3386/2/java/kudu-client/src/main/java/org/kududb/client/KuduTable.java
File java/kudu-client/src/main/java/org/kududb/client/KuduTable.java:
Line 143:* Use the {@link KuduScanToken} API instead of this method if
possible.
> Why would it not be possible to use the scan token API? Why haven't we depr
I would like to explicitly deprecate these methods. I initially didn't want to
wrap that into this bigger change, but I'll go ahead.
--
To view, visit http://gerrit.cloudera.org:8080/3386
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I78b5d400778547a9ee090111663677901dbadd98
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] [java-client] use tablet cache for locateTablet calls
Adar Dembo has posted comments on this change. Change subject: [java-client] use tablet cache for locateTablet calls .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/3386/2/java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java File java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java: Line 1876: private final List replicas = new ArrayList<>(); > I don't think the tradeoff is worth it. Maybe we should document this, but Makes sense to a degree, though bear in mind we're already doing DNS resolution with the lock held, and the overhead of a COW list copy is peanuts by comparison. If we began doing the resolution outside the lock, then yeah, the hit would be more sizable. -- To view, visit http://gerrit.cloudera.org:8080/3386 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I78b5d400778547a9ee090111663677901dbadd98 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
[kudu-CR] [java-client] use tablet cache for locateTablet calls
Jean-Daniel Cryans has posted comments on this change.
Change subject: [java-client] use tablet cache for locateTablet calls
..
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/3386/2/java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java
File java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java:
Line 1876: private final List replicas = new
ArrayList<>();
> Would it be safe to use a CopyOnWriteArrayList here, and in doing so avoid
I don't think the tradeoff is worth it. Maybe we should document this, but
replicas here would only be used when getting a list of locations. It's not a
common thing to do, and it's not typically done in conjunction with normal
inserts/scans.
Using CopyOnWriteArrayList means taking a hit under the lock while refreshing
the tablet clients, which happens a lot more often than getting the replicas.
Line 2047: List getReplicas() {
> Nit: may want to name it "getImmutableReplicas" so that callers know they c
Quickly looking through the code, looks like we don't do it, and I'm not sure
we even have occasions to do it. I like it too.
--
To view, visit http://gerrit.cloudera.org:8080/3386
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I78b5d400778547a9ee090111663677901dbadd98
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] [java-client] use tablet cache for locateTablet calls
Adar Dembo has posted comments on this change.
Change subject: [java-client] use tablet cache for locateTablet calls
..
Patch Set 2:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/3386/2//COMMIT_MSG
Commit Message:
PS2, Line 9: This simplifies and improves the performance of
AsyncKuduClient::locateTable by
: using the client's existing tablet cache.
Nit: there's not enough context in this patch to know why you're making this
change, especially given what you write below about nobody actually using these
APIs. I know the answer: it's because it means there's less code to write in
the non-covering partition patch that follows, but perhaps you can state that
explicitly as part of the commit description?
http://gerrit.cloudera.org:8080/#/c/3386/2/java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java
File java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java:
Line 1230: private final class MasterLookupCB implements Callback {
Nit: too long now? Unclear.
Line 1876: private final List replicas = new
ArrayList<>();
Would it be safe to use a CopyOnWriteArrayList here, and in doing so avoid the
need to hold any locks when accessing 'replicas'?
Line 2047: List getReplicas() {
Nit: may want to name it "getImmutableReplicas" so that callers know they can't
modify the list they are handed back.
It's a pleasant convention I remember from my CM days; I don't know whether
we're really following it in the Java client though.
http://gerrit.cloudera.org:8080/#/c/3386/2/java/kudu-client/src/main/java/org/kududb/client/KuduTable.java
File java/kudu-client/src/main/java/org/kududb/client/KuduTable.java:
Line 143:* Use the {@link KuduScanToken} API instead of this method if
possible.
Why would it not be possible to use the scan token API? Why haven't we
deprecated this family of methods explicitly?
--
To view, visit http://gerrit.cloudera.org:8080/3386
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I78b5d400778547a9ee090111663677901dbadd98
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] [java-client] use tablet cache for locateTablet calls
Jean-Daniel Cryans has posted comments on this change. Change subject: [java-client] use tablet cache for locateTablet calls .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/3386 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I78b5d400778547a9ee090111663677901dbadd98 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR] [java-client] use tablet cache for locateTablet calls
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/3386 to look at the new patch set (#2). Change subject: [java-client] use tablet cache for locateTablet calls .. [java-client] use tablet cache for locateTablet calls This simplifies and improves the performance of AsyncKuduClient::locateTable by using the client's existing tablet cache. The semantics of locateTable are changed slightly- the end key is now treated as an exclusive key instead of inclusive. This is a breaking change for the public KuduTable::locateTable methods. These methods are a holdover from the pre-scan token era, and should almost certainly be deprecated. To use correctly they require great knowledge of Kudu's internal partitioning logic. I'm not too worried about making this breaking change since the API is pretty much impossible to use to begin with, and no one is using it (except for the scan tokens impl, and it was already treating the end key as exclusive). Change-Id: I78b5d400778547a9ee090111663677901dbadd98 --- M docs/release_notes.adoc M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java M java/kudu-client/src/main/java/org/kududb/client/KuduTable.java M java/kudu-client/src/main/java/org/kududb/client/LocatedTablet.java M java/kudu-client/src/test/java/org/kududb/client/TestKuduTable.java 5 files changed, 119 insertions(+), 86 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/86/3386/2 -- To view, visit http://gerrit.cloudera.org:8080/3386 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I78b5d400778547a9ee090111663677901dbadd98 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [java-client] use tablet cache for locateTablet calls
Dan Burkert has posted comments on this change.
Change subject: [java-client] use tablet cache for locateTablet calls
..
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/3386/1/docs/release_notes.adoc
File docs/release_notes.adoc:
PS1, Line 63: Applications are
: encouraged to use the scan tokens API instead of these methods
in the future.
> Seems like you could also add that in KuduTable's javadoc for the methods a
Done
http://gerrit.cloudera.org:8080/#/c/3386/1/java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java
File java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java:
Line 1128: if (deadlineTracker.timedOut()) {
> Odd comment placement.
Done
Line 1927: for (Master.TabletLocationsPB.ReplicaPB replica :
tabletLocations.getReplicasList()) {
> Pretty sure this needs to be under the synchronized block above, else concu
Done
--
To view, visit http://gerrit.cloudera.org:8080/3386
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I78b5d400778547a9ee090111663677901dbadd98
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] [java-client] use tablet cache for locateTablet calls
Kudu Jenkins has posted comments on this change. Change subject: [java-client] use tablet cache for locateTablet calls .. Patch Set 2: Build Started http://104.196.14.100/job/kudu-gerrit/1834/ -- To view, visit http://gerrit.cloudera.org:8080/3386 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I78b5d400778547a9ee090111663677901dbadd98 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR] [java-client] use tablet cache for locateTablet calls
Jean-Daniel Cryans has posted comments on this change.
Change subject: [java-client] use tablet cache for locateTablet calls
..
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/3386/1/docs/release_notes.adoc
File docs/release_notes.adoc:
PS1, Line 63: Applications are
: encouraged to use the scan tokens API instead of these methods
in the future.
Seems like you could also add that in KuduTable's javadoc for the methods above.
http://gerrit.cloudera.org:8080/#/c/3386/1/java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java
File java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java:
Line 1128: // If the partition key location isn't cached, then kick off a
new tablet
Odd comment placement.
Line 1927: synchronized (replicas) {
Pretty sure this needs to be under the synchronized block above, else
concurrent metadata refreshes might leave you in a weird state.
--
To view, visit http://gerrit.cloudera.org:8080/3386
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I78b5d400778547a9ee090111663677901dbadd98
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] [java-client] use tablet cache for locateTablet calls
Kudu Jenkins has posted comments on this change. Change subject: [java-client] use tablet cache for locateTablet calls .. Patch Set 1: Build Started http://104.196.14.100/job/kudu-gerrit/1829/ -- To view, visit http://gerrit.cloudera.org:8080/3386 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I78b5d400778547a9ee090111663677901dbadd98 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR] [java-client] use tablet cache for locateTablet calls
Dan Burkert has uploaded a new change for review. http://gerrit.cloudera.org:8080/3386 Change subject: [java-client] use tablet cache for locateTablet calls .. [java-client] use tablet cache for locateTablet calls This simplifies and improves the performance of AsyncKuduClient::locateTable by using the client's existing tablet cache. The semantics of locateTable are changed slightly- the end key is now treated as an exclusive key instead of inclusive. This is a breaking change for the public KuduTable::locateTable methods. These methods are a holdover from the pre-scan token era, and should almost certainly be deprecated. To use correctly they require great knowledge of Kudu's internal partitioning logic. I'm not too worried about making this breaking change since the API is pretty much impossible to use to begin with, and no one is using it (except for the scan tokens impl, and it was already treating the end key as exclusive). Change-Id: I78b5d400778547a9ee090111663677901dbadd98 --- M docs/release_notes.adoc M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java M java/kudu-client/src/main/java/org/kududb/client/KuduTable.java M java/kudu-client/src/main/java/org/kududb/client/LocatedTablet.java M java/kudu-client/src/test/java/org/kududb/client/TestKuduTable.java 5 files changed, 109 insertions(+), 86 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/86/3386/1 -- To view, visit http://gerrit.cloudera.org:8080/3386 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I78b5d400778547a9ee090111663677901dbadd98 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert
