RATIS-2016. Correct NotificationInstallSnapshot's index#1030
RATIS-2016. Correct NotificationInstallSnapshot's index#1030szetszwo merged 4 commits intoapache:masterfrom
Conversation
szetszwo
left a comment
There was a problem hiding this comment.
@symious , thanks for filing this bug! Please see the comments inlined and also https://issues.apache.org/jira/secure/attachment/13066289/1030_review.patch
| pending.offer(request.getSnapshotChunk().getRequestIndex()); | ||
| pending.offer(isNotificationOnly ? INSTALL_SNAPSHOT_NOTIFICATION_INDEX | ||
| : request.getSnapshotChunk().getRequestIndex()); |
There was a problem hiding this comment.
Something is wrong in the existing code. When isNotificationOnly is true, it should have a notification but not a snapshotChunk. Protobuf will return SnapshotChunkProto.getDefaultInstance() when it does not have a snapshotChunk.
ratis/ratis-proto/src/main/proto/Raft.proto
Lines 224 to 227 in 5560718
There was a problem hiding this comment.
Let's check it more strictly:
final int index;
if (isNotificationOnly) {
Preconditions.assertSame(InstallSnapshotRequestBodyCase.NOTIFICATION,
request.getInstallSnapshotRequestBodyCase(), "request case");
index = 0;
} else {
Preconditions.assertSame(InstallSnapshotRequestBodyCase.SNAPSHOTCHUNK,
request.getInstallSnapshotRequestBodyCase(), "request case");
index = request.getSnapshotChunk().getRequestIndex();
}
if (index == 0) {
Preconditions.assertTrue(pending.isEmpty(), "pending queue is non-empty before offer for index 0");
}
pending.offer(index);| Preconditions.assertTrue(index == | ||
| (isNotificationOnly ? INSTALL_SNAPSHOT_NOTIFICATION_INDEX : reply.getRequestIndex())); | ||
|
|
There was a problem hiding this comment.
Similarly:
final int index = Objects.requireNonNull(pending.poll() , "index == null");
if (isNotificationOnly) {
Preconditions.assertSame(InstallSnapshotReplyBodyCase.SNAPSHOTINDEX,
reply.getInstallSnapshotReplyBodyCase(), "reply case");
Preconditions.assertSame(0, (int)index, "poll index");
} else {
Preconditions.assertSame(InstallSnapshotReplyBodyCase.REQUESTINDEX,
reply.getInstallSnapshotReplyBodyCase(), "reply case");
Preconditions.assertSame(reply.getRequestIndex(), (int)index, "poll index");
}
if (index == 0) {
Preconditions.assertTrue(pending.isEmpty(), "pending queue is non-empty after poll for index 0");
}|
@szetszwo Updated, PTAL. |
|
@symious , you are right. It is a queue but not a stack! |
szetszwo
left a comment
There was a problem hiding this comment.
+1 the change looks good.
|
@szetszwo Thank your for the review and merge. |
What changes were proposed in this pull request?
Currently the index of notify installSnapshot is decided by "
request.getSnapshotChunk().getRequestIndex()", which is valid because the proto is 0, this should be specified implicit to avoid confusion.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/RATIS-2016
How was this patch tested?
Original tests in
InstallSnapshotNotificationTestsshould be fine.