[kudu-CR] [java client] Extract ip2client out of AsyncKuduClient

[Jean-Daniel Cryans (Code Review)]

Jean-Daniel Cryans has submitted this change and it was merged.

Change subject: [java client] Extract ip2client out of AsyncKuduClient
..


[java client] Extract ip2client out of AsyncKuduClient

As part of making the Java client more modular and easier to test, this patch 
is moving
almost all of the connections management into a separate class. The change was 
rather
painless.

Change-Id: I48b0f3f262abd5ee26869202f661b3c25158f39c
Reviewed-on: http://gerrit.cloudera.org:8080/4717
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo 
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
A java/kudu-client/src/main/java/org/apache/kudu/client/ConnectionCache.java
M 
java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduSession.java
3 files changed, 417 insertions(+), 355 deletions(-)

Approvals:
  Adar Dembo: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I48b0f3f262abd5ee26869202f661b3c25158f39c
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

[kudu-CR] [java client] Extract ip2client out of AsyncKuduClient

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: [java client] Extract ip2client out of AsyncKuduClient
..


Patch Set 3: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I48b0f3f262abd5ee26869202f661b3c25158f39c
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: No

[kudu-CR] [java client] Extract ip2client out of AsyncKuduClient

[Jean-Daniel Cryans (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: [java client] Extract ip2client out of AsyncKuduClient
..

[java client] Extract ip2client out of AsyncKuduClient

As part of making the Java client more modular and easier to test, this patch 
is moving
almost all of the connections management into a separate class. The change was 
rather
painless.

Change-Id: I48b0f3f262abd5ee26869202f661b3c25158f39c
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
A java/kudu-client/src/main/java/org/apache/kudu/client/ConnectionCache.java
M 
java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduSession.java
3 files changed, 417 insertions(+), 355 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I48b0f3f262abd5ee26869202f661b3c25158f39c
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

[kudu-CR] [java client] Extract ip2client out of AsyncKuduClient

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: [java client] Extract ip2client out of AsyncKuduClient
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4717/1/java/kudu-client/src/main/java/org/apache/kudu/client/ConnectionCache.java
File java/kudu-client/src/main/java/org/apache/kudu/client/ConnectionCache.java:

PS1, Line 167: ,
> Done
The new sentence doesn't make sense though.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I48b0f3f262abd5ee26869202f661b3c25158f39c
Gerrit-PatchSet: 2
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] Extract ip2client out of AsyncKuduClient

[Jean-Daniel Cryans (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: [java client] Extract ip2client out of AsyncKuduClient
..

[java client] Extract ip2client out of AsyncKuduClient

As part of making the Java client more modular and easier to test, this patch 
is moving
almost all of the connections management into a separate class. The change was 
rather
painless.

Change-Id: I48b0f3f262abd5ee26869202f661b3c25158f39c
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
A java/kudu-client/src/main/java/org/apache/kudu/client/ConnectionCache.java
M 
java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduSession.java
3 files changed, 417 insertions(+), 355 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I48b0f3f262abd5ee26869202f661b3c25158f39c
Gerrit-PatchSet: 2
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] Extract ip2client out of AsyncKuduClient

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: [java client] Extract ip2client out of AsyncKuduClient
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4717/1/java/kudu-client/src/main/java/org/apache/kudu/client/ConnectionCache.java
File java/kudu-client/src/main/java/org/apache/kudu/client/ConnectionCache.java:

Line 91: this.kuduClient = client;
> Java is more advanced than you think ;)
I see. Nevertheless, circular references introduce unnecessary tight coupling 
and make it difficult to understand the underlying code.

I understand that this may not be something we can address immediately, but we 
should work to address it. I haven't looked at TabletClient much but we should 
factor out whatever AsyncKuduClient logic it uses into a separate class, then 
have both AsyncKuduClient and TabletClient (via ConnectionCache) depend on that 
class.


Line 279:   static String getIP(final String host) {
> It's used in both classes, so I'd rather keep the connection-related stuff 
Ah, didn't realize it was also being called from inside this class.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I48b0f3f262abd5ee26869202f661b3c25158f39c
Gerrit-PatchSet: 1
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] Extract ip2client out of AsyncKuduClient

[Jean-Daniel Cryans (Code Review)]

Jean-Daniel Cryans has posted comments on this change.

Change subject: [java client] Extract ip2client out of AsyncKuduClient
..


Patch Set 1:

(3 comments)

> (3 comments)
 > 
 > I looked at the ConnectionCache API and members, but only skimmed
 > the method implementations because I assumed that was just code
 > motion. Was there any implementation change worth looking at?

No.

http://gerrit.cloudera.org:8080/#/c/4717/1/java/kudu-client/src/main/java/org/apache/kudu/client/ConnectionCache.java
File java/kudu-client/src/main/java/org/apache/kudu/client/ConnectionCache.java:

Line 91: this.kuduClient = client;
> Isn't this a reference cycle between the AsyncKuduClient and ConnectionCach
Java is more advanced than you think ;)

Also the client itself is needed in TabletClientPipeline.


PS1, Line 167: AsyncKuduClient
> Odd to reference this class here, no?
Done


Line 279:   static String getIP(final String host) {
> Since this is static, I think it could remain in AsyncKuduClient, to keep t
It's used in both classes, so I'd rather keep the connection-related stuff in 
this class. AsyncKuduClient is already big enough.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I48b0f3f262abd5ee26869202f661b3c25158f39c
Gerrit-PatchSet: 1
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] Extract ip2client out of AsyncKuduClient

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: [java client] Extract ip2client out of AsyncKuduClient
..


Patch Set 1:

(3 comments)

I looked at the ConnectionCache API and members, but only skimmed the method 
implementations because I assumed that was just code motion. Was there any 
implementation change worth looking at?

http://gerrit.cloudera.org:8080/#/c/4717/1/java/kudu-client/src/main/java/org/apache/kudu/client/ConnectionCache.java
File java/kudu-client/src/main/java/org/apache/kudu/client/ConnectionCache.java:

Line 91: this.kuduClient = client;
Isn't this a reference cycle between the AsyncKuduClient and ConnectionCache, 
which would prevent the two from being GC'ed? I think it would be cleaner if 
you passed in the facilities that ConnectionCache needs (e.g. the timer, the 
channel factory, etc.). Then you wouldn't need getters for those on 
AsyncKuduClient either.


PS1, Line 167: AsyncKuduClient
Odd to reference this class here, no?


Line 279:   static String getIP(final String host) {
Since this is static, I think it could remain in AsyncKuduClient, to keep the 
ConnectionCache API slimmer.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I48b0f3f262abd5ee26869202f661b3c25158f39c
Gerrit-PatchSet: 1
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] Extract ip2client out of AsyncKuduClient

[Jean-Daniel Cryans (Code Review)]

Jean-Daniel Cryans has uploaded a new change for review.

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

Change subject: [java client] Extract ip2client out of AsyncKuduClient
..

[java client] Extract ip2client out of AsyncKuduClient

As part of making the Java client more modular and easier to test, this patch 
is moving
almost all of the connections management into a separate class. The change was 
rather
painless.

Change-Id: I48b0f3f262abd5ee26869202f661b3c25158f39c
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
A java/kudu-client/src/main/java/org/apache/kudu/client/ConnectionCache.java
M 
java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduSession.java
3 files changed, 417 insertions(+), 355 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/17/4717/1
-- 
To view, visit http://gerrit.cloudera.org:8080/4717
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I48b0f3f262abd5ee26869202f661b3c25158f39c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans