[
https://issues.apache.org/jira/browse/IGNITE-23834?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Roman Puchkovskiy updated IGNITE-23834:
---------------------------------------
Description:
All non-test implementations of RaftGroupListener behave identically with
respect to error handling. They have code like
} catch (IgniteInternalException e)
{ clo.result(e); }
catch (CompletionException e)
{ clo.result(e.getCause()); }
...
Calling clo.result() with an exception is not enough to let JRaft know that the
state machine might be broken; this just causes the error being logged and the
exception being sent to the client (if this is the leader).
As JRaft doesn't know that an error happened, it keeps applying subsequent
commands. If it so happens that command with index 10 is applied on node A, but
on B it throws an IgniteInternalException (and most of our exceptions are
subclasses of this class) or a CompletionException (which might wrap absolutely
anything), then a command with index 11 might still be applied on both nodes.
This would result in state machines' states diverging.
We should rethrow any throwable we catch by default to allow JRaft know about
it (if it sees such a throwable, it will mark the state machine as broken and
stop applying subsequent commands, which seems to be the only safe default
behavior). If, in the future, we realize that some exceptions might be normally
(also predictably based on the state machine state and command, and
idempotently) thrown, we should whitelist them individually. Probably, the best
way would be to create a specific exception class like
StateMachineLogicException. But currently, we don't seem to have such
exceptions at all, and by default we should just rethrow them to JRaft.
Another problem is that in all RaftGroupListener implementations we have the
following block in the very beginning of onWrite():
if (!busyLock.enterBusy())
{ iter.forEachRemaining(clo -> clo.result(new ShutdownException())); }
This means that we let the Raft client know that the command failed, but we
still apply all commands from the iterator. We should add a return in the end
of the block, but this will mean that we will confuse JRaft by making it think
that we applied the commands when we actually didn't. JRaft will then
erroneously increment appliedIndex. It is only persisted via the snapshots, so
we should also add busy lock usage to onSnapshotSave() implementations.
was:
All non-test implementations of RaftGroupListener behave identically with
respect to error handling. They have code like
} catch (IgniteInternalException e) {
clo.result(e);
} catch (CompletionException e){
clo.result(e.getCause());
}
...
Calling clo.result() with an exception is not enough to let JRaft know that the
state machine might be broken; this just causes the error being logged and the
exception being sent to the client (if this is the leader).
As JRaft doesn't know that an error happened, it keeps applying subsequent
commands. If it so happens that command with index 10 is applied on node A, but
on B it throws an IgniteInternalException (and most of our exceptions are
subclasses of this class) or a CompletionException (which might wrap absolutely
anything), then a command with index 11 might still be applied on both nodes.
This would result in state machines' states diverging.
We should rethrow any throwable we catch by default to allow JRaft know about
it (if it sees such a throwable, it will mark the state machine as broken and
stop applying subsequent commands, which seems to be the only safe default
behavior). If, in the future, we realize that some exceptions might be normally
(also predictably based on the state machine state and command, and
idempotently) thrown, we should whitelist them individually. Probably, the best
way would be to create a specific exception class like
StateMachineLogicException. But currently, we don't seem to have such
exceptions at all, and by default we should just rethrow them to JRaft.
Another problem is that in all RaftGroupListener implementations we have the
following block in the very beginning of onWrite():
if (!busyLock.enterBusy()) {
iter.forEachRemaining(clo -> clo.result(new ShutdownException()));
}
This means that we let the Raft client know that the command failed, but we
still apply all commands from the iterator. We should add a return in the end
of the block, but this will mean that we will confuse JRaft by making it think
that we applied the commands when we actually didn't. JRaft will then
erroneously
> RaftGroupListener implementations may silently break state machine state
> ------------------------------------------------------------------------
>
> Key: IGNITE-23834
> URL: https://issues.apache.org/jira/browse/IGNITE-23834
> Project: Ignite
> Issue Type: Bug
> Reporter: Roman Puchkovskiy
> Assignee: Roman Puchkovskiy
> Priority: Major
> Labels: ignite-3
> Time Spent: 10m
> Remaining Estimate: 0h
>
> All non-test implementations of RaftGroupListener behave identically with
> respect to error handling. They have code like
> } catch (IgniteInternalException e)
> { clo.result(e); }
> catch (CompletionException e)
> { clo.result(e.getCause()); }
> ...
> Calling clo.result() with an exception is not enough to let JRaft know that
> the state machine might be broken; this just causes the error being logged
> and the exception being sent to the client (if this is the leader).
> As JRaft doesn't know that an error happened, it keeps applying subsequent
> commands. If it so happens that command with index 10 is applied on node A,
> but on B it throws an IgniteInternalException (and most of our exceptions
> are subclasses of this class) or a CompletionException (which might wrap
> absolutely anything), then a command with index 11 might still be applied on
> both nodes. This would result in state machines' states diverging.
> We should rethrow any throwable we catch by default to allow JRaft know about
> it (if it sees such a throwable, it will mark the state machine as broken and
> stop applying subsequent commands, which seems to be the only safe default
> behavior). If, in the future, we realize that some exceptions might be
> normally (also predictably based on the state machine state and command, and
> idempotently) thrown, we should whitelist them individually. Probably, the
> best way would be to create a specific exception class like
> StateMachineLogicException. But currently, we don't seem to have such
> exceptions at all, and by default we should just rethrow them to JRaft.
>
> Another problem is that in all RaftGroupListener implementations we have the
> following block in the very beginning of onWrite():
> if (!busyLock.enterBusy())
> { iter.forEachRemaining(clo -> clo.result(new ShutdownException())); }
> This means that we let the Raft client know that the command failed, but we
> still apply all commands from the iterator. We should add a return in the end
> of the block, but this will mean that we will confuse JRaft by making it
> think that we applied the commands when we actually didn't. JRaft will then
> erroneously increment appliedIndex. It is only persisted via the snapshots,
> so we should also add busy lock usage to onSnapshotSave() implementations.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
