[kudu-CR] [java client] Cleanup AsyncKuduClient's unused caches

[Jean-Daniel Cryans (Code Review)]

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 
---
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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] [java client] Cleanup AsyncKuduClient's unused caches

[Adar Dembo (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] [java client] Cleanup AsyncKuduClient's unused caches

[Jean-Daniel Cryans (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] [java client] Cleanup AsyncKuduClient's unused caches

[Adar Dembo (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] [java client] Cleanup AsyncKuduClient's unused caches

[Jean-Daniel Cryans (Code Review)]

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 
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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] [java client] Cleanup AsyncKuduClient's unused caches

[Jean-Daniel Cryans (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] [java client] Cleanup AsyncKuduClient's unused caches

[Adar Dembo (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] [java client] Cleanup AsyncKuduClient's unused caches

[Dan Burkert (Code Review)]

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 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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] [java client] Cleanup AsyncKuduClient's unused caches

[Jean-Daniel Cryans (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] [java client] Cleanup AsyncKuduClient's unused caches

[Jean-Daniel Cryans (Code Review)]

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