Skip to content

RATIS-2505. Improve RATIS-2387 with direct synchronous append when compose disabled#1436

Merged
szetszwo merged 2 commits intoapache:masterfrom
ss77892:RATIS-2505
Apr 23, 2026
Merged

RATIS-2505. Improve RATIS-2387 with direct synchronous append when compose disabled#1436
szetszwo merged 2 commits intoapache:masterfrom
ss77892:RATIS-2505

Conversation

@ss77892
Copy link
Copy Markdown
Contributor

@ss77892 ss77892 commented Apr 20, 2026

What changes were proposed in this pull request?

When appendEntriesComposeEnabled is disabled (set to false), the code now bypasses the appendLog() method entirely and invokes the append operation directly on the current thread. This change eliminates the unnecessary async hop that remained in RATIS-2387. Instead of creating a completed future and then using thenComposeAsync() to schedule the append on the executor, the append now happens immediately on the calling thread.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/RATIS-2505

How was this patch tested?

Ozone cluster deployment with high volume write load.

@ss77892
Copy link
Copy Markdown
Contributor Author

ss77892 commented Apr 20, 2026

@szetszwo, you might find it interesting.

@ss77892 ss77892 requested a review from szetszwo April 20, 2026 16:21
Copy link
Copy Markdown
Contributor

@szetszwo szetszwo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change looks good. However, could you make it a one-line change?

+++ b/ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java
@@ -1682,7 +1682,7 @@ class RaftServerImpl implements RaftServer.Division,
     future.join();
     final CompletableFuture<Void> appendFuture = entries.isEmpty()? CompletableFuture.completedFuture(null)
         : appendLogTermIndices != null ? appendLogTermIndices.append(entries, this::appendLog)
-        : appendLog(entries);
+        : JavaUtils.allOf(state.getLog().append(entries));

     proto.getCommitInfosList().forEach(commitInfoCache::update);

Copy link
Copy Markdown

@yandrey321 yandrey321 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we cover these changes with unit tests?

@szetszwo
Copy link
Copy Markdown
Contributor

This revert the code to pre- RATIS-2235. So, new unit tests probably are not needed.

@ss77892
Copy link
Copy Markdown
Contributor Author

ss77892 commented Apr 21, 2026

@szetszwo sure. I just thought that if/else is easier to understand.

Copy link
Copy Markdown
Contributor

@szetszwo szetszwo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 the change looks good.

@szetszwo szetszwo merged commit 01b6e4f into apache:master Apr 23, 2026
18 checks passed
@szetszwo
Copy link
Copy Markdown
Contributor

... I just thought that if/else is easier to understand.

Different people have a different taste. If it is new code, it is fine either way. But for the existing code, let's just minimize the change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants