[kudu-CR] KUDU-1700: Debug build will not fail gracefully on Messenger::Init() failure

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: KUDU-1700: Debug build will not fail gracefully on 
Messenger::Init() failure
..


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4724/2/src/kudu/rpc/messenger.cc
File src/kudu/rpc/messenger.cc:

Line 106:   Status build_status = new_msgr->Init();
> warning: redundant get() call on smart pointer [google-readability-redundan
Done


Line 108: new_msgr->AllExternalReferencesDropped();
> warning: redundant get() call on smart pointer [google-readability-redundan
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id6021587c746af53305b3f601bb1bcc19f63eab0
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1700: Debug build will not fail gracefully on Messenger::Init() failure

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: KUDU-1700: Debug build will not fail gracefully on 
Messenger::Init() failure
..


Patch Set 3: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id6021587c746af53305b3f601bb1bcc19f63eab0
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] KUDU-1700: Debug build will not fail gracefully on Messenger::Init() failure

[Sailesh Mukil (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-1700: Debug build will not fail gracefully on 
Messenger::Init() failure
..

KUDU-1700: Debug build will not fail gracefully on Messenger::Init() failure

If Messenger::Init() fails in a debug build, a CHECK() will happen on
the destruction of the Messenger object. This happens beacuse, on an
error, the Messenger is not Shutdown().

This patch gets rid of the private function
Messenger::Build(Messenger**) and moves all its logic into the public
function Build(shared_ptr*). On an error, an explicit call
to AllExternalReferencesDropped() is made.

This case was probably never encountered as Messenger::Init()
currently has a very low chance of failing. However, in the future, as
more things may get added on to the function, this issue might show
up more often.

A more detailed explanation is given in the JIRA.

Change-Id: Id6021587c746af53305b3f601bb1bcc19f63eab0
---
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
2 files changed, 7 insertions(+), 11 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id6021587c746af53305b3f601bb1bcc19f63eab0
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [client.h] updated comment for CountBufferedOperations

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [client.h] updated comment for CountBufferedOperations
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4723/1/src/kudu/client/client.h
File src/kudu/client/client.h:

PS1, Line 1488:  
> nit: extra blank space.
Done


PS1, Line 1493: This is different than HasPendingOperations() above
> It makes you wonder if we want this method at all. On the Java side there's
Yep -- so far I can see, this method is used only in tests.  However, MJ for 
some reason was interested in this method as well.

Changing the name or dropping this method are both incompatible changes from 
the point of API compatibility.  If making such a change, I would vote for 
dropping this method (or at least declaring it as duplicate) if we are not sure 
whether it's useful at all.

Do you think we want to make such a change in 1.1.x?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If6293c0a3058f7056d24e7cee0120d1852ded548
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1700: Debug build will not fail gracefully on Messenger::Init() failure

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: KUDU-1700: Debug build will not fail gracefully on 
Messenger::Init() failure
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4724/1/src/kudu/rpc/messenger.cc
File src/kudu/rpc/messenger.cc:

PS1, Line 103: Status MessengerBuilder::Build(shared_ptr *msgr) {
 :   RETURN_NOT_OK(SaslInit(kSaslAppName)); // Initialize SASL 
library before we start making requests
 :   gscoped_ptr new_msgr(new Messenger(*this));
 :   Status build_status = new_msgr.get()->Init();
 :   if (!build_status.ok()) {
 : new_msgr.get()->AllExternalReferencesDropped();
 :  
> I think in the case of an error you'd want to just call AllExternalReferenc
Yes of course, don't know what I was thinking. I've merged both the functions 
now.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id6021587c746af53305b3f601bb1bcc19f63eab0
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1700: Debug build will not fail gracefully on Messenger::Init() failure

[Sailesh Mukil (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-1700: Debug build will not fail gracefully on 
Messenger::Init() failure
..

KUDU-1700: Debug build will not fail gracefully on Messenger::Init() failure

If Messenger::Init() fails in a debug build, a CHECK() will happen on
the destruction of the Messenger object. This happens beacuse, on an
error, the Messenger is not Shutdown().

This patch gets rid of the private function
Messenger::Build(Messenger**) and moves all its logic into the public
function Build(shared_ptr*). On an error, an explicit call
to AllExternalReferencesDropped() is made.

This case was probably never encountered as Messenger::Init()
currently has a very low chance of failing. However, in the future, as
more things may get added on to the function, this issue might show
up more often.

A more detailed explanation is given in the JIRA.

Change-Id: Id6021587c746af53305b3f601bb1bcc19f63eab0
---
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
2 files changed, 7 insertions(+), 11 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id6021587c746af53305b3f601bb1bcc19f63eab0
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-1700: Debug build will not fail gracefully on Messenger::Init() failure

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: KUDU-1700: Debug build will not fail gracefully on 
Messenger::Init() failure
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4724/1/src/kudu/rpc/messenger.cc
File src/kudu/rpc/messenger.cc:

PS1, Line 103: Status MessengerBuilder::Build(Messenger **msgr) {
 :   RETURN_NOT_OK(SaslInit(kSaslAppName)); // Initialize SASL 
library before we start making requests
 :   gscoped_ptr new_msgr(new Messenger(*this));
 :   *msgr = new_msgr.release();
 :   RETURN_NOT_OK((*msgr)->Init());
 :   return Status::OK();
 : }
> since this is just a private method called from one place, maybe we should 
Yes merging both of them makes sense to me.
But the other Build() will still require the Deleter to be added to it before 
returning it, on an error. Does that also count as something happening in the 
out-params?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id6021587c746af53305b3f601bb1bcc19f63eab0
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1700: Debug build will not fail gracefully on Messenger::Init() failure

[Sailesh Mukil (Code Review)]

Sailesh Mukil has uploaded a new change for review.

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

Change subject: KUDU-1700: Debug build will not fail gracefully on 
Messenger::Init() failure
..

KUDU-1700: Debug build will not fail gracefully on Messenger::Init() failure

If Messenger::Init() fails in a debug build, a CHECK() will happen on
the destruction of the Messenger object.
This patch reorders some code in the MessengerBuilder::Build()
functions to make sure that doesn't happen.

This case was probably never encountered as Messenger::Init()
currently has a very low chance of failing. However, in the future, as
more things may get added on to the function, this issue might show
up more often.

A more detailed explanation is given in the JIRA.

Change-Id: Id6021587c746af53305b3f601bb1bcc19f63eab0
---
M src/kudu/rpc/messenger.cc
1 file changed, 3 insertions(+), 3 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Id6021587c746af53305b3f601bb1bcc19f63eab0
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 

[kudu-CR] [client.h] updated comment for CountBufferedOperations

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: [client.h] updated comment for CountBufferedOperations
..

[client.h] updated comment for CountBufferedOperations

Change-Id: If6293c0a3058f7056d24e7cee0120d1852ded548
---
M src/kudu/client/client.h
1 file changed, 11 insertions(+), 4 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If6293c0a3058f7056d24e7cee0120d1852ded548
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 

[kudu-CR] [client.h] updated comment for CountBufferedOperations

[Jean-Daniel Cryans (Code Review)]

Jean-Daniel Cryans has posted comments on this change.

Change subject: [client.h] updated comment for CountBufferedOperations
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4723/1/src/kudu/client/client.h
File src/kudu/client/client.h:

PS1, Line 1488:  
nit: extra blank space.


PS1, Line 1493: This is different than HasPendingOperations() above
It makes you wonder if we want this method at all. On the Java side there's no 
equivalent. Maybe a better name would be "CountOperationsToFlush"?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If6293c0a3058f7056d24e7cee0120d1852ded548
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
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 (#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 RemoteTablet from AsyncKuduClient

[Jean-Daniel Cryans (Code Review)]

Jean-Daniel Cryans has posted comments on this change.

Change subject: [java client] Extract RemoteTablet from AsyncKuduClient
..


Patch Set 1:

(4 comments)

> (4 comments)
 > 
 > As with the other patch, has anything actually changed (besides
 > method visibility) in the moved code?

As you saw I moved the getting of a TabletClient directly into RemoteTablet, 
and that's about it.

http://gerrit.cloudera.org:8080/#/c/4719/1/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:

PS1, Line 589: tablet == null
> I presume that in the transition from clientFor() to tablet.getLeaderConnec
Yes.

So the tablet can be set to null to "invalidate" the scan, but looking through 
it I don't think it's possible to get here with a null tablet? I'll put an 
assert.


PS1, Line 689: getConnectionToLeader()
> This method doesn't exist.
Oops that got lost in the refactorings.


Line 1304:   rt.init(tabletPb);
> Can we do this as part of createTabletFromPb()? In fact, maybe createTablet
Can't do it in the ctor, it throws an exception.
I like the static RemoteTablet idea.


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

Line 74:   private final ConnectionCache connectionCache;
> Based on our discussion, sounds like this will go away because RemoteTablet
Right, this could go away at some point.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d06a573e4307c76a7aebab05cd5238fb0d9a2c6
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] [client.h] updated comment for CountBufferedOperations

[Alexey Serbin (Code Review)]

Alexey Serbin has uploaded a new change for review.

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

Change subject: [client.h] updated comment for CountBufferedOperations
..

[client.h] updated comment for CountBufferedOperations

Change-Id: If6293c0a3058f7056d24e7cee0120d1852ded548
---
M src/kudu/client/client.h
1 file changed, 15 insertions(+), 6 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: If6293c0a3058f7056d24e7cee0120d1852ded548
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 

[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] KUDU-1135 (part 2): avoid flushing metadata twice when starting an election

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: KUDU-1135 (part 2): avoid flushing metadata twice when starting 
an election
..


Patch Set 2: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I231273a1cfa92275788dd99c78e284ecd0543d7a
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] consensus: move more logic from ReplicaState to RaftConsensus

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: consensus: move more logic from ReplicaState to RaftConsensus
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4709/1/src/kudu/consensus/consensus_queue.h
File src/kudu/consensus/consensus_queue.h:

PS1, Line 257: bool IsCommittedIndexInCurrentTerm() const;
Is there an opportunity for the caller and the queue to disagree on what is the 
"current" term?


http://gerrit.cloudera.org:8080/#/c/4709/1/src/kudu/consensus/raft_consensus.cc
File src/kudu/consensus/raft_consensus.cc:

Line 618: 
DCHECK(round->replicate_msg()->change_config_record().has_old_config());
are these DCHECKs really necessary? old/new config are required fields of 
ChangeConfigRecordPB. We could likely replace this with just a DCHECK that the 
old/new config have the op id set below.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f377ebb132f3f58f984605197831f41198d78c5
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1135 (part 2): avoid flushing metadata twice when starting an election

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: KUDU-1135 (part 2): avoid flushing metadata twice when starting 
an election
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4702/2/src/kudu/consensus/raft_consensus.cc
File src/kudu/consensus/raft_consensus.cc:

Line 422:   // TODO(mpercy): Consider using a separate Mutex for voting, 
which must sync to disk.
> is this TODO invalid with the new mechanics?
I think it's still valid, since we haven't actually cleaned up the locking yet 
(even though it's two classes, all the code paths still hold the same locks as 
they used to)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I231273a1cfa92275788dd99c78e284ecd0543d7a
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] consensus: split ReplicaState in twain[1]

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: consensus: split ReplicaState in twain[1]
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4713/1/src/kudu/consensus/raft_consensus_state.h
File src/kudu/consensus/raft_consensus_state.h:

Line 71: class ReplicaState {
> should we rename this to ConsensusMetaState or something?
yea, wanted to defer that to another patch where we clean up the remainder of 
this class. In fact most of what remains is some locking and a bunch of 
"pass-through" methods to ConsensusMeta, so maybe this class isn't really 
necessary at all.

I guess I should update the class doc though.


Line 257: // Tracks the pending consensus rounds being managed by a Raft 
replica (either leader
> not your fault, but the mix of rounds, ops and transactions in method names
yea, I left a TODO below about this but let me move the TODO up to the top here 
about cleaning up naming throughout. I tried to minimize renames in this patch 
just to keep it easy to review.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I95308ae8a5d65ada016ae08e0e8cf06c54b35909
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1135 (part 2): avoid flushing metadata twice when starting an election

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: KUDU-1135 (part 2): avoid flushing metadata twice when starting 
an election
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4702/2/src/kudu/consensus/raft_consensus.cc
File src/kudu/consensus/raft_consensus.cc:

Line 422:   // TODO(mpercy): Consider using a separate Mutex for voting, 
which must sync to disk.
is this TODO invalid with the new mechanics?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I231273a1cfa92275788dd99c78e284ecd0543d7a
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: Yes

[kudu-CR] [java client] Extract RemoteTablet from AsyncKuduClient

[Jean-Daniel Cryans (Code Review)]

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

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

Change subject: [java client] Extract RemoteTablet from AsyncKuduClient
..

[java client] Extract RemoteTablet from AsyncKuduClient

RemoteTablet is responsible for handling the Java client's view of where 
replicas
are for its tablet, and who the leader is. Extracting this bit of functionality 
is
important if we want to be able to unit test it without bringing up a whole 
client
and possibly a cluster.

This patch makes a minor attempt at simplifying the interfaces, with more work 
to
come. Likewise for unit tests.

Change-Id: I3d06a573e4307c76a7aebab05cd5238fb0d9a2c6
---
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/KuduRpc.java
M java/kudu-client/src/main/java/org/apache/kudu/client/LocatedTablet.java
A java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java
M java/kudu-client/src/main/java/org/apache/kudu/client/TableLocationsCache.java
M java/kudu-client/src/main/java/org/apache/kudu/client/TabletClient.java
M 
java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduSession.java
8 files changed, 337 insertions(+), 309 deletions(-)


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

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

[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] 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] 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 

[kudu-CR] consensus queue: make methods non-virtual

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: consensus_queue: make methods non-virtual
..


Patch Set 1: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: If90af3a3db2a1c513f7e587dfbfa076699426803
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: No

[kudu-CR] consensus: move ReplicaTransactionFactory into RaftConsensus

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: consensus: move ReplicaTransactionFactory into RaftConsensus
..


Patch Set 1: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I45906913aed375f860276455223e43f0115a5c0a
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: No

[kudu-CR] consensus: remove bits of dead code

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: consensus: remove bits of dead code
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4712/1/src/kudu/consensus/raft_consensus.cc
File src/kudu/consensus/raft_consensus.cc:

Line 596:   CHECK(!s.IsServiceUnavailable()) << LogPrefixUnlocked() << "Log is 
shut down: " << s.ToString();
can we consolidate this check? seems like we're fataling here and below


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8dc0a129f4f136256083a31e4b4b27e6a673dbee
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1365. Add leader pre-elections

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: KUDU-1365. Add leader pre-elections
..


Patch Set 4:

| I guess we need to do something such that a peer with an old term bumps its 
term up to at least the term of the voters that respond. Any thoughts?

Yeah, that seems reasonable. If someone else has a higher term there's no point 
continuously trying to get elected with a lower one.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifcfabd8c9ffe31f17ab768542a046426f656db43
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
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] KUDU-1682. Lock contention on table locations cache in Java client

[Jean-Daniel Cryans (Code Review)]

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

Change subject: KUDU-1682. Lock contention on table locations cache in Java 
client
..


KUDU-1682. Lock contention on table locations cache in Java client

Change-Id: I0f6ba8f4fced6f043f7132fd11078044b004ea21
Reviewed-on: http://gerrit.cloudera.org:8080/4706
Reviewed-by: Adar Dembo 
Reviewed-by: Dan Burkert 
Tested-by: Kudu Jenkins
---
M java/kudu-client/src/main/java/org/apache/kudu/client/TableLocationsCache.java
1 file changed, 23 insertions(+), 6 deletions(-)

Approvals:
  Dan Burkert: Looks good to me, but someone else must approve
  Adar Dembo: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I0f6ba8f4fced6f043f7132fd11078044b004ea21
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] KUDU-1365. Add leader pre-elections

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: KUDU-1365. Add leader pre-elections
..


Patch Set 4:

Sorry, made a typo in my scenario. Peer A has term 1, so when it sends 
pre-elections, it's for term 2 (the current term of peer B).

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifcfabd8c9ffe31f17ab768542a046426f656db43
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No