szetszwo commented on code in PR #1047:
URL: https://github.com/apache/ratis/pull/1047#discussion_r1516661910
##########
ratis-common/src/main/java/org/apache/ratis/util/ProtoUtils.java:
##########
@@ -225,10 +226,20 @@ static String toString(CommitInfoProto proto) {
return RaftPeerId.valueOf(proto.getServer().getId()) + ":c" +
proto.getCommitIndex();
}
+ static String toString(LogInfoProto proto) {
+ return "lastSnapshot: " + proto.getLastSnapshot()
+ + ", applied: " + proto.getApplied()
+ + ", committed: " + proto.getCommitted();
+ }
+
static String toString(Collection<CommitInfoProto> protos) {
return
protos.stream().map(ProtoUtils::toString).collect(Collectors.toList()).toString();
}
+ static String toStringLogInfo(Collection<LogInfoProto> protos) {
+ return
protos.stream().map(ProtoUtils::toString).collect(Collectors.toList()).toString();
+ }
+
Review Comment:
These two `toString` methods are not used. Let's remove them?
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java:
##########
@@ -642,7 +643,22 @@ GroupInfoReply getGroupInfo(GroupInfoRequest request) {
final RaftConfigurationProto conf =
LogProtoUtils.toRaftConfigurationProtoBuilder(getRaftConf()).build();
return new GroupInfoReply(request, getCommitInfos(), getGroup(),
getRoleInfoProto(),
- dir.isHealthy(), conf);
+ dir.isHealthy(), conf, getLogInfo());
+ }
+
+ LogInfoProto getLogInfo(){
+ long currentTerm = getInfo().getCurrentTerm();
+ LogInfoProto.Builder logInfoBuilder = LogInfoProto.newBuilder();
+ logInfoBuilder.setApplied(TermIndex.valueOf(currentTerm,
getInfo().getLastAppliedIndex()).toProto());
Review Comment:
Use `getStateMachine().getLastAppliedTermIndex()`.
##########
ratis-proto/src/main/proto/Raft.proto:
##########
@@ -175,6 +175,15 @@ message CommitInfoProto {
uint64 commitIndex = 2;
}
+/** Add new LogInfoProto for RATIS-2030, allow GroupInfoCommand to show each
server's last committed log,
+ last applied log, last snapshot log, last entry log.*/
+message LogInfoProto {
+ TermIndexProto lastSnapshot = 1;
+ TermIndexProto applied = 2;
+ TermIndexProto committed = 3;
+ TermIndexProto lastEntry = 4;
+}
Review Comment:
Since `LogInfoProto` is only used in `GroupInfoReplyProto`. Let move it to
next to `GroupInfoReplyProto`.
##########
ratis-common/src/main/java/org/apache/ratis/protocol/GroupInfoReply.java:
##########
@@ -33,25 +34,27 @@ public class GroupInfoReply extends RaftClientReply {
private final RoleInfoProto roleInfoProto;
private final boolean isRaftStorageHealthy;
private final RaftConfigurationProto conf;
+ private LogInfoProto logInfoProto;
Review Comment:
add `final`.
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java:
##########
@@ -642,7 +643,22 @@ GroupInfoReply getGroupInfo(GroupInfoRequest request) {
final RaftConfigurationProto conf =
LogProtoUtils.toRaftConfigurationProtoBuilder(getRaftConf()).build();
return new GroupInfoReply(request, getCommitInfos(), getGroup(),
getRoleInfoProto(),
- dir.isHealthy(), conf);
+ dir.isHealthy(), conf, getLogInfo());
+ }
+
+ LogInfoProto getLogInfo(){
+ long currentTerm = getInfo().getCurrentTerm();
+ LogInfoProto.Builder logInfoBuilder = LogInfoProto.newBuilder();
+ logInfoBuilder.setApplied(TermIndex.valueOf(currentTerm,
getInfo().getLastAppliedIndex()).toProto());
+ logInfoBuilder.setCommitted(TermIndex.valueOf(currentTerm,
getRaftLog().getLastCommittedIndex()).toProto());
+
logInfoBuilder.setLastEntry(getRaftLog().getLastEntryTermIndex().toProto());
+ if (getStateMachine().getLatestSnapshot() != null) {
+ logInfoBuilder.setLastSnapshot(
+ TermIndex.valueOf(currentTerm,
+ getStateMachine().getLatestSnapshot().getIndex())
+ .toProto());
+ }
Review Comment:
Don't call the same method (`getLatestSnapshot()` in this case) multiple
times unless it is returning a constant (i.e. calling `getStateMachine()`
multiple times is okay):
1. They may return different values.
2. It may be expensive.
Also, use `snapshot.getTermIndex()`:
After all the changes, it should look like:
```java
LogInfoProto getLogInfo(){
final RaftLog log = getRaftLog();
final LogInfoProto.Builder logInfoBuilder = LogInfoProto.newBuilder()
.setApplied(getStateMachine().getLastAppliedTermIndex().toProto())
.setCommitted(log.getTermIndex(log.getLastCommittedIndex()).toProto())
.setLastEntry(log.getLastEntryTermIndex().toProto());
final SnapshotInfo snapshot = getStateMachine().getLatestSnapshot();
if (snapshot != null) {
logInfoBuilder.setLastSnapshot(snapshot.getTermIndex().toProto());
}
return logInfoBuilder.build();
}
```
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java:
##########
@@ -642,7 +643,22 @@ GroupInfoReply getGroupInfo(GroupInfoRequest request) {
final RaftConfigurationProto conf =
LogProtoUtils.toRaftConfigurationProtoBuilder(getRaftConf()).build();
return new GroupInfoReply(request, getCommitInfos(), getGroup(),
getRoleInfoProto(),
- dir.isHealthy(), conf);
+ dir.isHealthy(), conf, getLogInfo());
+ }
+
+ LogInfoProto getLogInfo(){
+ long currentTerm = getInfo().getCurrentTerm();
+ LogInfoProto.Builder logInfoBuilder = LogInfoProto.newBuilder();
+ logInfoBuilder.setApplied(TermIndex.valueOf(currentTerm,
getInfo().getLastAppliedIndex()).toProto());
+ logInfoBuilder.setCommitted(TermIndex.valueOf(currentTerm,
getRaftLog().getLastCommittedIndex()).toProto());
Review Comment:
We need to get the term from the log since the entry can be committed in an
earlier term.
```java
.setCommitted(log.getTermIndex(log.getLastCommittedIndex()).toProto())
```
--
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]
