You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2017/03/15 00:24:49 UTC

[kudu-CR] [java client] fixed NPE in master RPC error handler

Alexey Serbin has uploaded a new change for review.

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

Change subject: [java client] fixed NPE in master RPC error handler
......................................................................

[java client] fixed NPE in master RPC error handler

Fixed an NPE in dispatchMasterErrorOrReturnException().  The
AsyncKuduClient.handleNotLeader() is used to handle both tablet-
and master server errors.  In the latter case, the KuduRpc.tablet member
is null, so accessing it results in NPE with exception stack like below:

java.lang.NullPointerException
  at org.apache.kudu.client.AsyncKuduClient.handleNotLeader(AsyncKuduClient.java:1269)
  at org.apache.kudu.client.TabletClient.dispatchMasterErrorOrReturnException(TabletClient.java:536)
  at org.apache.kudu.client.TabletClient.messageReceived(TabletClient.java:461)
  at org.jboss.netty.channel.SimpleChannelUpstreamHandler.handleUpstream(SimpleChannelUpstreamHandler.java:70)
  at org.apache.kudu.client.TabletClient.handleUpstream(TabletClient.java:595)
  at org.jboss.netty.channel.DefaultChannelPipeline.sendUpstream(DefaultChannelPipeline.java:564)
  ...

The error has been spotted while running the
org.apache.kudu.client.TestRowErrors test.

Change-Id: Ib923212db1e0931ed63b31a39f03335c701f91cc
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
1 file changed, 6 insertions(+), 1 deletion(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib923212db1e0931ed63b31a39f03335c701f91cc
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>

[kudu-CR] WIP [java client] fixed NPE in master RPC error handler

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

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

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

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

Change subject: WIP [java client] fixed NPE in master RPC error handler
......................................................................

WIP [java client] fixed NPE in master RPC error handler

Fixed an NPE in dispatchMasterErrorOrReturnException().  The
AsyncKuduClient.handleNotLeader() is used to handle both tablet-
and master server errors.  In the latter case, the KuduRpc.tablet member
is null, so accessing it results in NPE with exception stack like below:

java.lang.NullPointerException
  at org.apache.kudu.client.AsyncKuduClient.handleNotLeader(AsyncKuduClient.java:1269)
  at org.apache.kudu.client.TabletClient.dispatchMasterErrorOrReturnException(TabletClient.java:536)
  at org.apache.kudu.client.TabletClient.messageReceived(TabletClient.java:461)
  at org.jboss.netty.channel.SimpleChannelUpstreamHandler.handleUpstream(SimpleChannelUpstreamHandler.java:70)
  at org.apache.kudu.client.TabletClient.handleUpstream(TabletClient.java:595)
  at org.jboss.netty.channel.DefaultChannelPipeline.sendUpstream(DefaultChannelPipeline.java:564)
  ...

The error has been spotted while running the
org.apache.kudu.client.TestRowErrors test.

WIP:
  This NPE happens only if ConnectoToMaster RPC returns
  NOT_THE_LEADER error. From the other side, fixing this NPE does not
  resolve the situation: another NPE is thrown then:
java.lang.NullPointerException
  at org.apache.kudu.client.AsyncKuduClient.sendRpcToTablet(AsyncKuduClient.java:737)
  at org.apache.kudu.client.AsyncKuduClient$2RetryTimer.run(AsyncKuduClient.java:1302)
  at org.jboss.netty.util.HashedWheelTimer$HashedWheelTimeout.expire(HashedWheelTimer.java:556)
  ...

  So, this requires additional clarification.

Change-Id: Ib923212db1e0931ed63b31a39f03335c701f91cc
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestMasterFailover.java
M src/kudu/master/master_service.cc
3 files changed, 27 insertions(+), 1 deletion(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib923212db1e0931ed63b31a39f03335c701f91cc
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] [java client] fixed NPE in master RPC error handler

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.

Change subject: [java client] fixed NPE in master RPC error handler
......................................................................


Patch Set 1:

> The minicluster options should be able to be configured
 > programaticaly on a per-test-case basis (eg check how the security
 > test does it). Then we can add a new suite which adds the
 > appropriate stress tests here. I don't think Maven profiles are the
 > way to do it (too hard to run, etc)

I found that the error happens only if master server responds with NOT_THE_LEADER error on ConnectToMaster RPC.  I tried different scenarios, injecting that errors in different master RPC calls and provoking frequent leader re-elections, but that does not trigger this NPE.

However, even having this NPE fixed, injecting NOT_THE_LEADER into ConnectoToMaster() response brings in another:
java.lang.NullPointerException
	at org.apache.kudu.client.AsyncKuduClient.sendRpcToTablet(AsyncKuduClient.java:737)
	at org.apache.kudu.client.AsyncKuduClient$2RetryTimer.run(AsyncKuduClient.java:1302)

I'm not sure that's the right way to continue here -- need to clarify on this.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib923212db1e0931ed63b31a39f03335c701f91cc
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] WIP [java client] fixed NPE in master RPC error handler

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.

Change subject: WIP [java client] fixed NPE in master RPC error handler
......................................................................


Patch Set 2:

> Uploaded patch set 2.

That's still a WIP patch.  After spending some time and looking at that, it seems that's not the only place to patch the issue.  From the other side, if not returning NOT_THE_LEADER error from ConnectToMaster RPC, this NPE does not happen, even if with frequent leader re-elections, etc.  In other words, it's necessary to clarify on this before continuing.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib923212db1e0931ed63b31a39f03335c701f91cc
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] [java client] fixed NPE in master RPC error handler

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.

Change subject: [java client] fixed NPE in master RPC error handler
......................................................................


Patch Set 1:

The minicluster options should be able to be configured programaticaly on a per-test-case basis (eg check how the security test does it). Then we can add a new suite which adds the appropriate stress tests here. I don't think Maven profiles are the way to do it (too hard to run, etc)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib923212db1e0931ed63b31a39f03335c701f91cc
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] WIP [java client] fixed NPE in master RPC error handler

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.

Change subject: WIP [java client] fixed NPE in master RPC error handler
......................................................................


Patch Set 2: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib923212db1e0931ed63b31a39f03335c701f91cc
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] [java client] fixed NPE in master RPC error handler

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.

Change subject: [java client] fixed NPE in master RPC error handler
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6401/1//COMMIT_MSG
Commit Message:

Line 24: org.apache.kudu.client.TestRowErrors test.
any ideas how we might provoke it to happen more reliably? Are we lacking test coverage of master failover?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib923212db1e0931ed63b31a39f03335c701f91cc
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] [java client] fixed NPE in master RPC error handler

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.

Change subject: [java client] fixed NPE in master RPC error handler
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6401/1//COMMIT_MSG
Commit Message:

Line 24: org.apache.kudu.client.TestRowErrors test.
> any ideas how we might provoke it to happen more reliably? Are we lacking t
Sure -- this issue became apparent when testing against changes for KUDU-1927 (https://gerrit.cloudera.org/#/c/6356/)

I think we can provoke it if having test like I wrote for KUDU-1927.  Perhaps, we should just run an external minicluster with those parameters while running Java tests.  I don't know what is the best way of doing that -- just some additional maven profile for the project?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib923212db1e0931ed63b31a39f03335c701f91cc
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] WIP [java client] fixed NPE in master RPC error handler

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has abandoned this change.

Change subject: WIP [java client] fixed NPE in master RPC error handler
......................................................................


Abandoned

Partially addressed by bee8e875e21acb1d36f104f8388fc9ed6c963821, and in the long run this should be fixed by other means avoiding the 'masterTable' hack.

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

Gerrit-MessageType: abandon
Gerrit-Change-Id: Ib923212db1e0931ed63b31a39f03335c701f91cc
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>