sumitagrawl commented on code in PR #1218:
URL: https://github.com/apache/ratis/pull/1218#discussion_r1949065594
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/StateMachineUpdater.java:
##########
@@ -182,19 +180,20 @@ public String toString() {
@Override
public void run() {
+ CompletableFuture<Void> applyLogFutures =
CompletableFuture.completedFuture(null);
for(; state != State.STOP; ) {
try {
- waitForCommit();
+ waitForCommit(applyLogFutures);
Review Comment:
Trying to understand changes and impact,
1. waitForCoimmit(applyLogFutures) --> call takeSnapshot(applyLogFutures)
--> future.get() ==> No Ops
2. applyLog(applyLogFutures): can add some future object to be wait
3. checkAndTakeSnapshot(applyLogFutures) : This is moved before STOP(), and
does future.get() by internal takeSnapshot()
4. In Stop() check, just do future.get() for waiting task completion which
was not there earlier
Q1: Step 3 changes checkAndTakeSnapshot() moving out of stop() and force
checking to take snapshot do have performance impact?
Q2: Let No snapshot to be taken, in this case, future.get() is never called,
do this is intended ?
- wait on future is called if need take snapshot in checkAndTakeSnapshot()
- wait on future is called if stop()
- else its not called
Q3: We can refactor code as above behavior, that,
- applyLog() can return existing future set
- check for future.get() before stop()
- move checkAndTakeSnapshot() out before stop()
This have similar impact instead of passing future deep inside
takeSnapshot().
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/StateMachineUpdater.java:
##########
@@ -182,19 +180,20 @@ public String toString() {
@Override
public void run() {
+ CompletableFuture<Void> applyLogFutures =
CompletableFuture.completedFuture(null);
for(; state != State.STOP; ) {
try {
- waitForCommit();
+ waitForCommit(applyLogFutures);
if (state == State.RELOAD) {
reload();
}
- final MemoizedSupplier<List<CompletableFuture<Message>>> futures =
applyLog();
- checkAndTakeSnapshot(futures);
+ applyLogFutures = applyLog(applyLogFutures);
+ checkAndTakeSnapshot(applyLogFutures);
Review Comment:
In one of case where no snpashot to be taken and no stop(), future.get()
will not be called. Do this will fix the issue as expected for applyTransaction
to finish ? I think no, it should wait for all case.
And related to performance impact if always need wait, do this call is
always in stop() flow or all flow ? this part is not clear.
--
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]
