RATIS-2387. Performance degradation after RATIS-2235#1337
RATIS-2387. Performance degradation after RATIS-2235#1337szetszwo merged 2 commits intoapache:masterfrom
Conversation
szetszwo
left a comment
szetszwo
left a comment
There was a problem hiding this comment.
@ss77892 , thanks a lot for working on this! Please see the comments inlined and also https://issues.apache.org/jira/secure/attachment/13080467/1337_review.patch
| this.appendEntriesSynchronized = Boolean.parseBoolean( | ||
| System.getProperty("raft.server.log.append.entries.synchronized", "true")); | ||
| LOG.info("{}: appendLog synchronization mode: {}", getMemberId(), | ||
| appendEntriesSynchronized ? "synchronized" : "parallel"); |
There was a problem hiding this comment.
Use system property for single-file configuration (avoids updating ratis-server-api jar)
This conf should be added to RaftServerConfigKeys but not adding a new way for setting it.
For the property name, let's name it "raft.server.log.append-entries.compose.enabled". BTW, appendEntries is always synchronized. The change of RATIS-2235 was to compose the previous future.
| private final AtomicReference<CompletableFuture<Void>> appendLogFuture; | ||
| private final NavigableIndices appendLogTermIndices = new NavigableIndices(); | ||
| private final NavigableIndices appendLogTermIndices; | ||
| private final boolean appendEntriesSynchronized; |
There was a problem hiding this comment.
Let's simply check if appendLogTermIndices == null instead of adding a new boolean.
| : appendLog(entries); | ||
|
|
||
| // Conditional appendLog based on configuration (RATIS-2235) | ||
| final CompletableFuture<Void> appendOperation; |
There was a problem hiding this comment.
Let's call it appendFuture.
| appendOperation = appendLogSynchronized(entries); | ||
| } else { | ||
| final List<CompletableFuture<Long>> futures = entries.isEmpty() ? Collections.emptyList() | ||
| : state.getLog().append(entries); |
There was a problem hiding this comment.
We should run it with serverExecutor.
| * Synchronized appendLog operation to ensure only one thread performs appendLog at a time. | ||
| * This is the RATIS-2235 implementation that can be enabled via configuration. | ||
| */ | ||
| private CompletableFuture<Void> appendLogSynchronized(List<LogEntryProto> entries) { |
There was a problem hiding this comment.
After this change, this method and appendLogFuture are only used when it is enabled. Let's move them to NavigableIndices.
|
It is actually your idea/code. I post the file just for the review. If you like the suggestions, could you update this PR? |
There was a problem hiding this comment.
The changes in this file are unrelated. Please revert this file. (BTW, using Optional is not good for performance since it unnecessarily creates objects.)
Sorry, accidentally got a WIP patch for another Jira applied. Fixed that. |
szetszwo
left a comment
szetszwo
left a comment
There was a problem hiding this comment.
+1 the change looks good.
2 participants
What changes were proposed in this pull request?
RATIS-2235 made append processing synchronized. That made Apache Ozone write operations slower (up to 30% for relatively slow drives). We haven't seen any issues with parallel processing, so it might be reasonable to make it optional.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/RATIS-2387
How was this patch tested?
Ozone benchmark tests (freon).