RATIS-2009. ReferenceCount should work for all LogEntry types. #1021
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Author
|
@szetszwo can you have a look? |
…ing submitted to RaftServer by SlidingWindow.
Contributor
Author
|
I also just attempted to solve the following test failure in this PR. @szetszwo
|
szetszwo
reviewed
Jan 19, 2024
Contributor
szetszwo
left a comment
szetszwo
left a comment
There was a problem hiding this comment.
Thanks for fixing also testWatchRequestAsyncChangeLeader! Please see the comments inlined.
Comment on lines
130
to
135
| default CompletableFuture<Long> appendEntry(LogEntryProto entry, TransactionContext context) { | ||
| return appendEntry(entry); | ||
| return appendEntry(ReferenceCountedObject.wrap(entry), context); | ||
| } | ||
|
|
||
| CompletableFuture<Long> appendEntry(ReferenceCountedObject<LogEntryProto> entryRef, TransactionContext context); | ||
|
|
Contributor
There was a problem hiding this comment.
Since this is in ratis-server-api, we have to provide a default implementation of the new method for compatibility.
/**
* Append asynchronously an entry.
* Used by the leader.
*/
default CompletableFuture<Long> appendEntry(ReferenceCountedObject<LogEntryProto> entryRef, TransactionContext context) {
return appendEntry(entryRef.get(), context);
}
/**
* @deprecated use {@link #append(ReferenceCountedObject)}.
*/
@Deprecated
default CompletableFuture<Long> appendEntry(LogEntryProto entry, TransactionContext context) {
throw new UnsupportedOperationException();
}
Contributor
There was a problem hiding this comment.
Update also RaftLogBase:
@@ -345,15 +346,17 @@ public abstract class RaftLogBase implements RaftLog {
@Override
public final CompletableFuture<Long> appendEntry(LogEntryProto entry) {
- return appendEntry(entry, null);
+ return appendEntry(ReferenceCountedObject.wrap(entry), null);
}
| final long seq = pending.getSeqNum(); | ||
| processClientRequest(pending.getRequestRef(), | ||
| reply -> slidingWindow.receiveReply(seq, reply, this::sendReply)); | ||
| pending.release(); |
Contributor
There was a problem hiding this comment.
We should use whenComplete(..).
final ReferenceCountedObject<RaftClientRequest> ref = pending.getRequestRef();
processClientRequest(ref, reply -> slidingWindow.receiveReply(seq, reply, this::sendReply))
.whenComplete((v, e) -> ref.release());
Contributor
Author
There was a problem hiding this comment.
The processClientRequest eventually calls submitClientRequestAsync(ReferenceCountedObject<RaftClientRequest>) that returns a Future.
I realized that when calling any asynchronous method accepting a reference counter, we hardly need to wait for whenComplete to release the counter. Look at an example below
final RaftClientRequest request = requestRef.retain();
try {
// do something with the request.
// call asynchronous
return submitClientRequestAsync()
// We don't need and should not do this
// .whenComplete((r, e) -> requestRef.release());
;
} finally {
// instead just release the counter here.
requestRef.release()
}
This is because:
- Inside
submitClientRequestAsync, there's alreadyretainwhen needed (in LogSegment cache). Other words, if the internal logic ofsubmitClientRequestAsyncneeds to extend the life of the referenced object, that should be done there. The outside client code responsibility is to ensure the reference isretainablewhen it reaches thesubmitClientRequestAsyncand this is done by the outsideretain. The outside code releases the counter as soon as it finishes using it. - There're cases that
whenCompleteis not called. For example, when client times out or disconects, the return future seems to be discarded (documented per RATIS-2007).
Comment on lines
70
to
72
| void release() { | ||
| requestRef.release(); | ||
| } |
Contributor
There was a problem hiding this comment.
This method could be removed.
szetszwo
reviewed
Jan 19, 2024
Contributor
szetszwo
left a comment
szetszwo
left a comment
There was a problem hiding this comment.
@duongkame , thanks for the update! Just a minor comment inlined.
Comment on lines
126
to
133
| /** | ||
| * Append asynchronously an entry. | ||
| * Used by the leader. | ||
| * Used by the leader for scenarios that there is no needs to clean up resources when the given entry is no | ||
| * longer used/referenced by this log. | ||
| */ | ||
| default CompletableFuture<Long> appendEntry(LogEntryProto entry, TransactionContext context) { | ||
| return appendEntry(entry); | ||
| return appendEntry(ReferenceCountedObject.wrap(entry), context); | ||
| } |
Contributor
There was a problem hiding this comment.
The method is not used anywhere except for the new append(ReferenceCountedObject). It should throw an exception and annotated as deprecated.
/**
* @deprecated use {@link #append(ReferenceCountedObject)}.
*/
@Deprecated
default CompletableFuture<Long> appendEntry(LogEntryProto entry, TransactionContext context) {
throw new UnsupportedOperationException();
}
szetszwo
approved these changes
Jan 19, 2024
Contributor
szetszwo
left a comment
szetszwo
left a comment
There was a problem hiding this comment.
+1 the change looks good.
symious
pushed a commit
to symious/ratis
that referenced
this pull request
Apr 4, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
See RATIS-2009
Found this issue when running tests for #1014, TestServerRestartWithGrpc failed because RaftConfiguration entry zero-copy buffer are released too early (while the log entry are still in the EntryCache.)
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/RATIS-2009
How was this patch tested?
Existing tests.