[
https://issues.apache.org/jira/browse/IGNITE-11459?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Sergey Chugunov updated IGNITE-11459:
-------------------------------------
Description:
Working on IGNITE-11364 I found the following suspicious detail about
StatusCheck flow: in the message there is a special field {{failedNodeId}}
which seems to duplicate functionality of {{failedNodes}} collection in
TcpDiscoveryAbstractMessage.
{{failedNodeId}} field is filled only in special scenario of failed ping or
remote node. It is used *only* to ignore the message.
Historical overview of this field revealed commit *838с0fd* where a meaningful
piece of code was either intentionally removed or accidentally lost:
{noformat}
if (msg instanceof TcpDiscoveryStatusCheckMessage) {
TcpDiscoveryStatusCheckMessage msg0 = (TcpDiscoveryStatusCheckMessage)msg;
if (next.id().equals(msg0.failedNodeId())) {
next = null;
if (log.isDebugEnabled())
log.debug("Discarding status check since next node has indeed
failed [next=" + next +
", msg=" + msg + ']');
// Discard status check message by exiting loop and handle failure.
break;
}
}
{noformat}
Conclusion: field {{failedNodeId}} and the whole flow around it looks
suspicious and has to be reviewed for flaws. Review should result in either
redesign of the flow or deleting the code.
was:
Working on IGNITE-11364 I found the following suspicious detail about
StatusCheck flow: in the message there is a special field {{failedNodeId}}
which seems to duplicate functionality of {{failedNodes}} collection in
TcpDiscoveryAbstractMessage.
{{failedNodeId}} field is filled only in special scenario of failed ping or
remote node. It is used *only* to ignore the message.
Historical overview of this field revealed commit *838с0fd* where a meaningful
piece of code was either intentionally removed or accidentally lost:
{noformat}
if (msg instanceof TcpDiscoveryStatusCheckMessage) {
TcpDiscoveryStatusCheckMessage msg0 =
(TcpDiscoveryStatusCheckMessage)msg;
if (next.id().equals(msg0.failedNodeId())) {
next = null;
if (log.isDebugEnabled())
log.debug("Discarding status check since next
node has indeed failed [next=" + next +
", msg=" + msg + ']');
// Discard status check message by exiting loop and
handle failure.
break;
}
}
{noformat}
Conclusion: field {{failedNodeId}} and the whole flow around it looks
suspicious and has to be reviewed for flaws. Review should result in either
redesign of the flow or deleting the code.
> Possible dead code in TcpDiscoveryStatusCheckMessage flow
> ---------------------------------------------------------
>
> Key: IGNITE-11459
> URL: https://issues.apache.org/jira/browse/IGNITE-11459
> Project: Ignite
> Issue Type: Improvement
> Affects Versions: 2.7
> Reporter: Sergey Chugunov
> Priority: Minor
>
> Working on IGNITE-11364 I found the following suspicious detail about
> StatusCheck flow: in the message there is a special field {{failedNodeId}}
> which seems to duplicate functionality of {{failedNodes}} collection in
> TcpDiscoveryAbstractMessage.
> {{failedNodeId}} field is filled only in special scenario of failed ping or
> remote node. It is used *only* to ignore the message.
> Historical overview of this field revealed commit *838с0fd* where a
> meaningful piece of code was either intentionally removed or accidentally
> lost:
> {noformat}
> if (msg instanceof TcpDiscoveryStatusCheckMessage) {
> TcpDiscoveryStatusCheckMessage msg0 = (TcpDiscoveryStatusCheckMessage)msg;
> if (next.id().equals(msg0.failedNodeId())) {
> next = null;
> if (log.isDebugEnabled())
> log.debug("Discarding status check since next node has indeed
> failed [next=" + next +
> ", msg=" + msg + ']');
> // Discard status check message by exiting loop and handle failure.
> break;
> }
> }
> {noformat}
> Conclusion: field {{failedNodeId}} and the whole flow around it looks
> suspicious and has to be reviewed for flaws. Review should result in either
> redesign of the flow or deleting the code.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
