-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[IOTDB-1583] Raft log failed to be committed in cluster version #3832
Conversation
@@ -1032,6 +1034,15 @@ TSStatus processPlanLocally(PhysicalPlan plan) { | |||
log.setCurrLogTerm(getTerm().get()); | |||
log.setCurrLogIndex(logManager.getLastLogIndex() + 1); | |||
|
|||
// if a single log exceeds the threshold | |||
// we need to return error code to the client as in server mode | |||
if ((int) RamUsageEstimator.sizeOf(log) + Integer.BYTES |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RamUsageEstimator.sizeOf() is larger than the serialized size of the log, this check may have a risk
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. Let me update it. But serializing here probably means that each log will be serialized many times.
With the current structure of the code, there seems to be no better solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed at offline, status code of the error could be refined later.
logger.error( | ||
"Log cannot fit into buffer, please increase raft_log_buffer_size;" | ||
+ "or reduce the size of requests you send."); | ||
return StatusUtils.INTERNAL_ERROR; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like EXECUTE_STATEMENT_ERROR should be a more proper status code to tell user.
@@ -1032,6 +1034,15 @@ TSStatus processPlanLocally(PhysicalPlan plan) { | |||
log.setCurrLogTerm(getTerm().get()); | |||
log.setCurrLogIndex(logManager.getLastLogIndex() + 1); | |||
|
|||
// if a single log exceeds the threshold | |||
// we need to return error code to the client as in server mode | |||
if ((int) RamUsageEstimator.sizeOf(log) + Integer.BYTES |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The buffer size to be used by the object is the bytes size after serialize the object. I don't think the memory size of a object is equals to its serialized bytes size in most of cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
>= ClusterDescriptor.getInstance().getConfig().getRaftLogBufferSize()) { | ||
logger.error( | ||
"Log cannot fit into buffer, please increase raft_log_buffer_size;" | ||
+ "or reduce the size of requests you send."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- We need to control the log size. We can't let him have this anomaly.
- If an exception occurs, the server obtains the exception and then splits the log file or uses other methods to apply the log file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- We need to control the log size. We can't let him have this anomaly.
- If an exception occurs, the server obtains the exception and then splits the log file or uses other methods to apply the log file.
When this problem occurs, the service can only be restarted. And the current modification method is consistent with the single server mode. So the operation of splitting logs involves several modules, we may need to control the plan size but not only log.
We may need to spend some time on this part in the future to make it better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this needs further discussion, is some error happened during raft log apply
, maybe only one raft group failed, it will lose some data, and cause the raft group not consistent with each other, how to deal with the exception?
For now, I think it ok just throw the exception and not let the log be committed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, Thanks for submit a new issues for further discussion in jira, this pr could merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all your suggesions,I will merge the pr, further discussion can be found here #3856
The bug is caused by a raft log is oversize.
When the size of a log is bigger than buffer (default 16MB),it can not be persistent due to the
BufferOverflowException
. But the log has committed in memory, we will commit the same log again.So,we should check the size of the log at the entrance to avoid that leader will send this log to followers.
BTW, this case may cause the next plan failed in server mode. Because we didn't clear the buffer, it still contains some information from the previous log.