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
HDDS-10546. OM startup failure as leader is not getting ready #6395
Conversation
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.
@sumitagrawl , thanks for working on this! The change looks good. Just two minor comments inlined.
// if newTermIndex getting updated is within sequence of notifiedTermIndex (i.e. from lastSkippedIndex and | ||
// notifiedTermIndex), then can update directly to lastNotifiedTermIndex as it ensure previous double buffer's | ||
// Index is notified or getting notified matching lastSkippedIndex | ||
if (newTermIndex.getIndex() <= getLastNotifiedTermIndex().getIndex() |
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.
Use <
instead of <=
// notifiedTermIndex), then can update directly to lastNotifiedTermIndex as it ensure previous double buffer's | ||
// Index is notified or getting notified matching lastSkippedIndex | ||
if (newTermIndex.getIndex() <= getLastNotifiedTermIndex().getIndex() | ||
&& getLastAppliedTermIndex().getIndex() >= lastSkippedIndex) { |
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.
Don't call getLastAppliedTermIndex()
twice, i.e.
@Override
protected synchronized boolean updateLastAppliedTermIndex(TermIndex newTermIndex) {
final TermIndex lastApplied = getLastAppliedTermIndex();
assertUpdateIncreasingly("lastApplied", lastApplied, newTermIndex);
// if newTermIndex getting updated is within sequence of notifiedTermIndex (i.e. from lastSkippedIndex and
// notifiedTermIndex), then can update directly to lastNotifiedTermIndex as it ensure previous double buffer's
// Index is notified or getting notified matching lastSkippedIndex
if (newTermIndex.getIndex() < getLastNotifiedTermIndex().getIndex()
&& lastApplied.getIndex() >= lastSkippedIndex) {
newTermIndex = getLastNotifiedTermIndex();
}
return super.updateLastAppliedTermIndex(newTermIndex);
}
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.
+1 the change looks good.
@sumitagrawl Thanks for the patch, @szetszwo for the review. |
What changes were proposed in this pull request?
OM is not getting stopped even after Leader is elected but Leader is not ready. This is an impact of HDDS-10026.
As solution, need update lastAppliedTransactionId with notifiedIndex if that is in sequence. So that Ratis can also get latest sequenceId as updated.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-10546
How was this patch tested?