szetszwo commented on code in PR #1030:
URL: https://github.com/apache/ratis/pull/1030#discussion_r1467943809
##########
ratis-grpc/src/main/java/org/apache/ratis/grpc/server/GrpcLogAppender.java:
##########
@@ -577,15 +579,19 @@ private class InstallSnapshotResponseHandler implements
StreamObserver<InstallSn
void addPending(InstallSnapshotRequestProto request) {
try (AutoCloseableLock writeLock = lock.writeLock(caller, LOG::trace)) {
- pending.offer(request.getSnapshotChunk().getRequestIndex());
+ pending.offer(isNotificationOnly ? INSTALL_SNAPSHOT_NOTIFICATION_INDEX
+ : request.getSnapshotChunk().getRequestIndex());
Review 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`.
https://github.com/apache/ratis/blob/5560718aba3ec9cb1447f7305ba4e23b2567effa/ratis-proto/src/main/proto/Raft.proto#L224-L227
##########
ratis-grpc/src/main/java/org/apache/ratis/grpc/server/GrpcLogAppender.java:
##########
@@ -577,15 +579,19 @@ private class InstallSnapshotResponseHandler implements
StreamObserver<InstallSn
void addPending(InstallSnapshotRequestProto request) {
try (AutoCloseableLock writeLock = lock.writeLock(caller, LOG::trace)) {
- pending.offer(request.getSnapshotChunk().getRequestIndex());
+ pending.offer(isNotificationOnly ? INSTALL_SNAPSHOT_NOTIFICATION_INDEX
+ : request.getSnapshotChunk().getRequestIndex());
+
}
}
void removePending(InstallSnapshotReplyProto reply) {
try (AutoCloseableLock writeLock = lock.writeLock(caller, LOG::trace)) {
final Integer index = pending.poll();
Objects.requireNonNull(index, "index == null");
- Preconditions.assertTrue(index == reply.getRequestIndex());
+ Preconditions.assertTrue(index ==
+ (isNotificationOnly ? INSTALL_SNAPSHOT_NOTIFICATION_INDEX :
reply.getRequestIndex()));
+
Review Comment:
Similarly:
```java
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");
}
```
##########
ratis-grpc/src/main/java/org/apache/ratis/grpc/server/GrpcLogAppender.java:
##########
@@ -577,15 +579,19 @@ private class InstallSnapshotResponseHandler implements
StreamObserver<InstallSn
void addPending(InstallSnapshotRequestProto request) {
try (AutoCloseableLock writeLock = lock.writeLock(caller, LOG::trace)) {
- pending.offer(request.getSnapshotChunk().getRequestIndex());
+ pending.offer(isNotificationOnly ? INSTALL_SNAPSHOT_NOTIFICATION_INDEX
+ : request.getSnapshotChunk().getRequestIndex());
Review Comment:
Let's check it more strictly:
```java
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);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
