lokeshj1703 commented on a change in pull request #88:
URL: https://github.com/apache/incubator-ratis/pull/88#discussion_r420700281
##########
File path:
ratis-grpc/src/main/java/org/apache/ratis/grpc/server/GrpcLogAppender.java
##########
@@ -369,13 +369,22 @@ public void onNext(InstallSnapshotReplyProto reply) {
switch (reply.getResult()) {
case SUCCESS:
+ LOG.info("{}: Completed InstallSnapshot. Reply: {}", this);
+ removePending(reply);
+ break;
+ case NOTIFIED:
Review comment:
Based on my understanding I think IN_PROGRESS and NOTIFIED are similar.
NOTIFIED means that follower received the request? If this is the case can we
use IN_PROGRESS to reply once follower receives the request?
##########
File path:
ratis-grpc/src/main/java/org/apache/ratis/grpc/server/GrpcLogAppender.java
##########
@@ -369,13 +369,22 @@ public void onNext(InstallSnapshotReplyProto reply) {
switch (reply.getResult()) {
case SUCCESS:
+ LOG.info("{}: Completed InstallSnapshot. Reply: {}", this);
Review comment:
We need to add reply in the arguments.
##########
File path:
ratis-grpc/src/main/java/org/apache/ratis/grpc/server/GrpcLogAppender.java
##########
@@ -369,13 +369,22 @@ public void onNext(InstallSnapshotReplyProto reply) {
switch (reply.getResult()) {
case SUCCESS:
+ LOG.info("{}: Completed InstallSnapshot. Reply: {}", this);
+ removePending(reply);
+ break;
+ case NOTIFIED:
+ LOG.info("{}: Notified to InstallSnapshot.", this);
+ removePending(reply);
+ break;
case IN_PROGRESS:
+ LOG.info("{}: InstallSnapshot in progress.", this);
removePending(reply);
break;
case ALREADY_INSTALLED:
final long followerSnapshotIndex = reply.getSnapshotIndex();
- LOG.info("{}: set follower snapshotIndex to {}.", this,
followerSnapshotIndex);
+ LOG.info("{}: Already Installed Snapshot Index {}.", this,
followerSnapshotIndex);
getFollower().setSnapshotIndex(followerSnapshotIndex);
+ updateCommitIndex(followerSnapshotIndex);
Review comment:
Should we also update nextIndex?
##########
File path:
ratis-grpc/src/main/java/org/apache/ratis/grpc/server/GrpcLogAppender.java
##########
@@ -394,17 +403,19 @@ public void onNext(InstallSnapshotReplyProto reply) {
@Override
public void onError(Throwable t) {
if (!isAppenderRunning()) {
- LOG.info("{} is stopped", this);
+ LOG.info("{} is stopped", GrpcLogAppender.this);
return;
}
- LOG.error("{}: Failed installSnapshot: {}", this, t);
+ GrpcUtil.warn(LOG, () -> this + ": Failed InstallSnapshot", t);
+ grpcServerMetrics.onRequestRetry(); // Update try counter
resetClient(null);
- close();
Review comment:
We can add a flag in close to mark it as closed without notifying append
in this case.
##########
File path:
ratis-grpc/src/main/java/org/apache/ratis/grpc/server/GrpcLogAppender.java
##########
@@ -394,17 +403,19 @@ public void onNext(InstallSnapshotReplyProto reply) {
@Override
public void onError(Throwable t) {
if (!isAppenderRunning()) {
- LOG.info("{} is stopped", this);
+ LOG.info("{} is stopped", GrpcLogAppender.this);
return;
}
- LOG.error("{}: Failed installSnapshot: {}", this, t);
+ GrpcUtil.warn(LOG, () -> this + ": Failed InstallSnapshot", t);
+ grpcServerMetrics.onRequestRetry(); // Update try counter
resetClient(null);
- close();
}
@Override
public void onCompleted() {
- LOG.info("{}: follower responses installSnapshot COMPLETED", this);
+ if (!firstResponseReceived) {
Review comment:
Why do we need this check?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
