You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org> on 2016/10/12 23:11:06 UTC
[kudu-CR] [java client] Cleanup AsyncKuduClient's unused caches
Jean-Daniel Cryans has uploaded a new change for review.
http://gerrit.cloudera.org:8080/4705
Change subject: [java client] Cleanup AsyncKuduClient's unused caches
......................................................................
[java client] Cleanup AsyncKuduClient's unused caches
Originally, asynchbase came with a few caches such as server-connection->region and
region->server-connection. Via dozens of patches, we've reduced their usefulness to 0.
This patch simple finishes the job and removes them.
FWIW client2tablets was always a source of pain, and the caching logic is now a lot
easier to manage, and potentially refactor!
Change-Id: I62802c34c618c83a4ff69d79825387cbe4ab51a8
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduSession.java
2 files changed, 10 insertions(+), 61 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/05/4705/1
--
To view, visit http://gerrit.cloudera.org:8080/4705
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I62802c34c618c83a4ff69d79825387cbe4ab51a8
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans <jd...@apache.org>
[kudu-CR] [java client] Cleanup AsyncKuduClient's unused caches
Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.
Jean-Daniel Cryans has posted comments on this change.
Change subject: [java client] Cleanup AsyncKuduClient's unused caches
......................................................................
Patch Set 4:
> So no regression test for that race?
Sorry, where in this gerrit did you mention a regression test? :P
TBH right now it'd be a pain. Right now I'm doing even more refactoring, I think testing will soon become a lot easier.
--
To view, visit http://gerrit.cloudera.org:8080/4705
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I62802c34c618c83a4ff69d79825387cbe4ab51a8
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] [java client] Cleanup AsyncKuduClient's unused caches
Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.
Jean-Daniel Cryans has posted comments on this change.
Change subject: [java client] Cleanup AsyncKuduClient's unused caches
......................................................................
Patch Set 3:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/4705/2/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:
PS2, Line 144: /**
: * This map contains data cached from calls to the master's
: * GetTableLocations RPC. This map is keyed by table ID.
: */
: private final ConcurrentHashMap<String, TableLocationsCache> tableLocations =
:
> Nit: just combine the two paragraphs, like "This map contains data cached f
Done
Line 178:
> Could you add a "GuardedBy" annotation to this?
Done
Line 1360: Slice tabletId = rt.tabletId;
> This comment is probably unnecessary now.
Done
Line 2008: String ip = getIP(host);
> If this has to be called with tabletServers synchronized, then it should be
Done
--
To view, visit http://gerrit.cloudera.org:8080/4705
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I62802c34c618c83a4ff69d79825387cbe4ab51a8
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] [java client] Cleanup AsyncKuduClient's unused caches
Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.
Jean-Daniel Cryans has uploaded a new patch set (#2).
Change subject: [java client] Cleanup AsyncKuduClient's unused caches
......................................................................
[java client] Cleanup AsyncKuduClient's unused caches
Originally, asynchbase came with a few caches such as server-connection->region and
region->server-connection. Via dozens of patches, we've reduced their usefulness to 0.
This patch simple finishes the job and removes them.
FWIW client2tablets was always a source of pain, and the caching logic is now a lot
easier to manage, and potentially refactor!
Change-Id: I62802c34c618c83a4ff69d79825387cbe4ab51a8
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduSession.java
2 files changed, 18 insertions(+), 74 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/05/4705/2
--
To view, visit http://gerrit.cloudera.org:8080/4705
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I62802c34c618c83a4ff69d79825387cbe4ab51a8
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [java client] Cleanup AsyncKuduClient's unused caches
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [java client] Cleanup AsyncKuduClient's unused caches
......................................................................
Patch Set 4:
> > So no regression test for that race?
>
> Sorry, where in this gerrit did you mention a regression test? :P
I didn't mention it in the gerrit, but it's what we discussed on Slack. I assumed you'd pair it with this patch since without the patch, this new test would fail.
> TBH right now it'd be a pain. Right now I'm doing even more
> refactoring, I think testing will soon become a lot easier.
Okay.
--
To view, visit http://gerrit.cloudera.org:8080/4705
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I62802c34c618c83a4ff69d79825387cbe4ab51a8
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] [java client] Cleanup AsyncKuduClient's unused caches
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: [java client] Cleanup AsyncKuduClient's unused caches
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/4705/2/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 178: final HashMap<String, TabletClient> ip2client = new HashMap<>();
Could you add a "GuardedBy" annotation to this?
Line 2008: void addTabletClient(String uuid, String host, int port, boolean isLeader)
If this has to be called with tabletServers synchronized, then it should be private, since tabletServers is private. Also, remove the comment and add a GuardedBy annotation.
--
To view, visit http://gerrit.cloudera.org:8080/4705
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I62802c34c618c83a4ff69d79825387cbe4ab51a8
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] [java client] Cleanup AsyncKuduClient's unused caches
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [java client] Cleanup AsyncKuduClient's unused caches
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/4705/2/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:
PS2, Line 144: /**
: * This map contains data cached from calls to the master's
: * GetTableLocations RPC.
: *
: * This map is keyed by table ID.
: */
Nit: just combine the two paragraphs, like "This map contains data cached from calls to the master's GetTableLocations RPC. It is keyed by table ID."
Line 1360: // Early creating the tablet so that it parses out the pb.
This comment is probably unnecessary now.
--
To view, visit http://gerrit.cloudera.org:8080/4705
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I62802c34c618c83a4ff69d79825387cbe4ab51a8
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] [java client] Cleanup AsyncKuduClient's unused caches
Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.
Jean-Daniel Cryans has submitted this change and it was merged.
Change subject: [java client] Cleanup AsyncKuduClient's unused caches
......................................................................
[java client] Cleanup AsyncKuduClient's unused caches
Originally, asynchbase came with a few caches such as server-connection->region and
region->server-connection. Via dozens of patches, we've reduced their usefulness to 0.
This patch simple finishes the job and removes them.
FWIW client2tablets was always a source of pain, and the caching logic is now a lot
easier to manage, and potentially refactor!
Change-Id: I62802c34c618c83a4ff69d79825387cbe4ab51a8
Reviewed-on: http://gerrit.cloudera.org:8080/4705
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <ad...@cloudera.com>
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduSession.java
2 files changed, 22 insertions(+), 80 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/4705
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I62802c34c618c83a4ff69d79825387cbe4ab51a8
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [java client] Cleanup AsyncKuduClient's unused caches
Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4705
to look at the new patch set (#3).
Change subject: [java client] Cleanup AsyncKuduClient's unused caches
......................................................................
[java client] Cleanup AsyncKuduClient's unused caches
Originally, asynchbase came with a few caches such as server-connection->region and
region->server-connection. Via dozens of patches, we've reduced their usefulness to 0.
This patch simple finishes the job and removes them.
FWIW client2tablets was always a source of pain, and the caching logic is now a lot
easier to manage, and potentially refactor!
Change-Id: I62802c34c618c83a4ff69d79825387cbe4ab51a8
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduSession.java
2 files changed, 22 insertions(+), 80 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/05/4705/3
--
To view, visit http://gerrit.cloudera.org:8080/4705
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I62802c34c618c83a4ff69d79825387cbe4ab51a8
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [java client] Cleanup AsyncKuduClient's unused caches
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [java client] Cleanup AsyncKuduClient's unused caches
......................................................................
Patch Set 4: Code-Review+2
So no regression test for that race?
--
To view, visit http://gerrit.cloudera.org:8080/4705
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I62802c34c618c83a4ff69d79825387cbe4ab51a8
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No