You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@kudu.apache.org by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org> on 2016/09/12 05:05:45 UTC

[kudu-CR] WIP: [java] - Synchronize the removal of the TabletClient from client2tablets and ip2client

David Ribeiro Alves has uploaded a new change for review.

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

Change subject: WIP: [java] - Synchronize the removal of the TabletClient from client2tablets and ip2client
......................................................................

WIP: [java] - Synchronize the removal of the TabletClient from client2tablets and ip2client

When entries are added to the maps they are added under a synchronized block, but
since entries where never removed from client2tablets the removal was never synchronized.
This makes it so that we remove the entries from both maps under the same lock we use
to add them.

Change-Id: I86197aed50be52588c3debe590d660c709cddacd
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
1 file changed, 13 insertions(+), 21 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I86197aed50be52588c3debe590d660c709cddacd
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>

[kudu-CR] WIP: [java] - Synchronize the removal of the TabletClient from client2tablets and ip2client

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.

David Ribeiro Alves has posted comments on this change.

Change subject: WIP: [java] - Synchronize the removal of the TabletClient from client2tablets and ip2client
......................................................................


Patch Set 1: Verified+1

unrelated failure org.apache.kudu.client.ITClient.test

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I86197aed50be52588c3debe590d660c709cddacd
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] [java] - Synchronize the removal of the TabletClient from client2tablets and ip2client

Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.

Kudu Jenkins has posted comments on this change.

Change subject: [java] - Synchronize the removal of the TabletClient from client2tablets and ip2client
......................................................................


Patch Set 2:

Build Started http://104.196.14.100/job/kudu-gerrit/3369/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I86197aed50be52588c3debe590d660c709cddacd
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] [java] - Synchronize the removal of the TabletClient from client2tablets and ip2client

Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.

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

Change subject: [java] - Synchronize the removal of the TabletClient from client2tablets and ip2client
......................................................................


[java] - Synchronize the removal of the TabletClient from client2tablets and ip2client

When entries are added to the maps they are added under a synchronized block, but
since entries where never removed from client2tablets the removal was never synchronized.
This was causing an error under concurrency where a disconnect would remove the client
from both maps, but a simultaneous master lookup would not see the removal in ip2client
and thus expect that client2tablets still have the entry, which was no longer the case.

This makes it so that we remove the entries from both maps under the same lock we use
to add them.

I've ran the test 100 times without failures.

Change-Id: I86197aed50be52588c3debe590d660c709cddacd
Reviewed-on: http://gerrit.cloudera.org:8080/4380
Tested-by: Kudu Jenkins
Reviewed-by: Jean-Daniel Cryans <jd...@apache.org>
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
1 file changed, 13 insertions(+), 21 deletions(-)

Approvals:
  Jean-Daniel Cryans: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I86197aed50be52588c3debe590d660c709cddacd
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] [java] - Synchronize the removal of the TabletClient from client2tablets and ip2client

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.

David Ribeiro Alves has uploaded a new patch set (#2).

Change subject: [java] - Synchronize the removal of the TabletClient from client2tablets and ip2client
......................................................................

[java] - Synchronize the removal of the TabletClient from client2tablets and ip2client

When entries are added to the maps they are added under a synchronized block, but
since entries where never removed from client2tablets the removal was never synchronized.
This was causing an error under concurrency where a disconnect would remove the client
from both maps, but a simultaneous master lookup would not see the removal in ip2client
and thus expect that client2tablets still have the entry, which was no longer the case.

This makes it so that we remove the entries from both maps under the same lock we use
to add them.

I've ran the test 100 times without failures.

Change-Id: I86197aed50be52588c3debe590d660c709cddacd
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
1 file changed, 13 insertions(+), 21 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I86197aed50be52588c3debe590d660c709cddacd
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>

[kudu-CR] [java] - Synchronize the removal of the TabletClient from client2tablets and ip2client

Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.

Jean-Daniel Cryans has posted comments on this change.

Change subject: [java] - Synchronize the removal of the TabletClient from client2tablets and ip2client
......................................................................


Patch Set 2:

(1 comment)

How did you hit this originally? Obviously if it's hitting an NPE then there's a problem and we should fix it... Could use some cluster testing too.

http://gerrit.cloudera.org:8080/#/c/4380/2//COMMIT_MSG
Commit Message:

Line 18: I've ran the test 100 times without failures.
Was it failing before?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I86197aed50be52588c3debe590d660c709cddacd
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes