[jira] [Comment Edited] (RATIS-337) In RaftServerImpl, leaderState/heartbeatMonitor may be accessed without proper null check

["Tsz Wo Nicholas Sze (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/RATIS-337?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16641350#comment-16641350 ] 

Tsz Wo Nicholas Sze edited comment on RATIS-337 at 10/8/18 5:01 AM:
--------------------------------------------------------------------

Thanks, [~jnp] for the comments.

> ... Do you see major perf impact if checkLeaderState is synchronized in RaftServerImpl?

checkLeaderState(..) is called in the first few statements in submitClientRequestAsync(..).  If it is synchronized, it will hurt the performance.

> As long as commit-index is updated, this will happen in the background, is this really needed at shutting down of leader thread?

It seems okay. I just have copied the TODO from the earlier code.  Let me think about it in details.

> This method doesn't need to be static. 

It is better to be static -- it is clear that it does not use any fields in the class.  Let me move it to ServerProtoUtils.


was (Author: szetszwo):
Thanks, [~jnp] for the comments.

> ... Do you see major perf impact if checkLeaderState is synchronized in RaftServerImpl?

checkLeaderState(..) is called in the first few statements in submitClientRequestAsync(..).  If it is synchronized, it will hurt the performance.

> As long as commit-index is updated, this will happen in the background, is this really needed at shutting down of leader thread?

I just have copied the TODO from the earlier code.  Let me think about it in details.

> This method doesn't need to be static. 

It is better to be static -- it is clear that it does not use any fields in the class.  Let me move it to ServerProtoUtils.

> In RaftServerImpl, leaderState/heartbeatMonitor may be accessed without proper null check
> -----------------------------------------------------------------------------------------
>
>                 Key: RATIS-337
>                 URL: https://issues.apache.org/jira/browse/RATIS-337
>             Project: Ratis
>          Issue Type: Bug
>          Components: server
>            Reporter: Tsz Wo Nicholas Sze
>            Assignee: Tsz Wo Nicholas Sze
>            Priority: Major
>         Attachments: r337_20181008.patch
>
>
> leaderState/heartbeatMonitor is declared as volatile. Some code like below won't work since leaderState may be set to null in between.
> {code:java}
> //RaftServerImpl.checkLeaderState(..)
>     } else if (leaderState == null || !leaderState.isReady()) {
> {code}



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)