You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ratis.apache.org by "SzyWilliam (via GitHub)" <gi...@apache.org> on 2025/01/02 14:48:20 UTC
[PR] RATIS-2235. Allow only one thread to perform appendLog [ratis]
SzyWilliam opened a new pull request, #1206:
URL: https://github.com/apache/ratis/pull/1206
See https://issues.apache.org/jira/browse/RATIS-2235
See the discussion at https://issues.apache.org/jira/browse/RATIS-2208
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2235. Allow only one thread to perform appendLog [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on PR #1206:
URL: https://github.com/apache/ratis/pull/1206#issuecomment-2569941453
The test failures seem related. Not yet sure what is the problem.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2235. Allow only one thread to perform appendLog [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo merged PR #1206:
URL: https://github.com/apache/ratis/pull/1206
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2235. Allow only one thread to perform appendLog [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #1206:
URL: https://github.com/apache/ratis/pull/1206#discussion_r1901220350
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java:
##########
@@ -1641,9 +1645,11 @@ leaderId, getMemberId(), currentTerm, followerCommit, inconsistencyReplyNextInde
state.updateConfiguration(entries);
}
future.join();
-
- final List<CompletableFuture<Long>> futures = entries.isEmpty() ? Collections.emptyList()
+ final CompletableFuture<Void> appendLog = appendLogFuture.updateAndGet(f -> f.thenCompose(ignored -> {
+ final List<CompletableFuture<Long>> futures = entries.isEmpty() ? Collections.emptyList()
: state.getLog().append(requestRef.delegate(entries));
+ return JavaUtils.allOf(futures);
+ }));
Review Comment:
We should retain the ref first, i.e.
```java
@@ -1641,9 +1645,8 @@ class RaftServerImpl implements RaftServer.Division,
state.updateConfiguration(entries);
}
future.join();
-
- final List<CompletableFuture<Long>> futures = entries.isEmpty() ? Collections.emptyList()
- : state.getLog().append(requestRef.delegate(entries));
+ final CompletableFuture<Void> appendLog = entries.isEmpty()? appendLogFuture.get()
+ : appendLog(requestRef.delegate(entries));
proto.getCommitInfosList().forEach(commitInfoCache::update);
CodeInjectionForTesting.execute(LOG_SYNC, getId(), null);
@@ -1657,7 +1660,7 @@ class RaftServerImpl implements RaftServer.Division,
final long commitIndex = effectiveCommitIndex(proto.getLeaderCommit(), previous, entries.size());
final long matchIndex = isHeartbeat? RaftLog.INVALID_LOG_INDEX: entries.get(entries.size() - 1).getIndex();
- return JavaUtils.allOf(futures).whenCompleteAsync((r, t) -> {
+ return appendLog.whenCompleteAsync((r, t) -> {
followerState.ifPresent(fs -> fs.updateLastRpcTime(FollowerState.UpdateType.APPEND_COMPLETE));
timer.stop();
}, getServerExecutor()).thenApply(v -> {
@@ -1675,6 +1678,13 @@ class RaftServerImpl implements RaftServer.Division,
});
}
+ private CompletableFuture<Void> appendLog(ReferenceCountedObject<List<LogEntryProto>> entriesRef) {
+ entriesRef.retain();
+ return appendLogFuture.updateAndGet(f -> f.thenCompose(
+ ignored -> JavaUtils.allOf(state.getLog().append(entriesRef))))
+ .whenComplete((v, e) -> entriesRef.release());
+ }
+
private long checkInconsistentAppendEntries(TermIndex previous, List<LogEntryProto> entries) {
// Check if a snapshot installation through state machine is in progress.
final long installSnapshot = snapshotInstallationHandler.getInProgressInstallSnapshotIndex();
```
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2235. Allow only one thread to perform appendLog [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #1206:
URL: https://github.com/apache/ratis/pull/1206#discussion_r1902255725
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java:
##########
@@ -1641,9 +1645,9 @@ leaderId, getMemberId(), currentTerm, followerCommit, inconsistencyReplyNextInde
state.updateConfiguration(entries);
}
future.join();
+ final CompletableFuture<Void> appendLog = entries.isEmpty()? appendLogFuture.get()
Review Comment:
@SzyWilliam , I see the problem now -- when it is empty, it should not wait for the previous append. Otherwise, it blocks the heartbeats.
```java
final CompletableFuture<Void> appendLog = entries.isEmpty()? CompletableFuture.completedFuture(null)
```
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2235. Allow only one thread to perform appendLog [ratis]
Posted by "SzyWilliam (via GitHub)" <gi...@apache.org>.
SzyWilliam commented on PR #1206:
URL: https://github.com/apache/ratis/pull/1206#issuecomment-2573141345
@szetszwo the tests are all passed now, PTAL ~
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2235. Allow only one thread to perform appendLog [ratis]
Posted by "SzyWilliam (via GitHub)" <gi...@apache.org>.
SzyWilliam commented on code in PR #1206:
URL: https://github.com/apache/ratis/pull/1206#discussion_r1904125313
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java:
##########
@@ -1641,9 +1645,9 @@ leaderId, getMemberId(), currentTerm, followerCommit, inconsistencyReplyNextInde
state.updateConfiguration(entries);
}
future.join();
+ final CompletableFuture<Void> appendLog = entries.isEmpty()? appendLogFuture.get()
Review Comment:
That is a tricky one. Thanks for spotting and troubleshooting this!
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2235. Allow only one thread to perform appendLog [ratis]
Posted by "SzyWilliam (via GitHub)" <gi...@apache.org>.
SzyWilliam commented on PR #1206:
URL: https://github.com/apache/ratis/pull/1206#issuecomment-2569357596
@szetszwo Thanks for reviewing this pr and help with the code. Now I learned how to use zero-copy in async programming, thank you!
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2235. Allow only one thread to perform appendLog [ratis]
Posted by "SzyWilliam (via GitHub)" <gi...@apache.org>.
SzyWilliam commented on PR #1206:
URL: https://github.com/apache/ratis/pull/1206#issuecomment-2575141354
@szetszwo Thanks for reviewing and merging this PR!
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org