RATIS-2625. Streaming: support sending commands in Data Stream - #1534
Conversation
|
@szetszwo this is the follow up for apache/ozone#10823 (comment) |
There was a problem hiding this comment.
@amaliujia , thanks for working on this! The change looks quite good.
The StateMachine may return a result, we should send it back to the client; see https://issues.apache.org/jira/secure/attachment/13083627/1534_review2.patch
| * @param streamOffset the current stream byte offset | ||
| * @return a future for the command task | ||
| */ | ||
| default CompletableFuture<?> onCommand(ByteBuffer command, long streamOffset) { |
There was a problem hiding this comment.
Since this is a commnad, it should return CompletableFuture<ByteBuffer>.
szetszwo
left a comment
There was a problem hiding this comment.
@amaliujia , thanks for the update! Please see the comments inlined.
|
|
||
| @Override | ||
| public CompletableFuture<DataStreamReply> commandAsync(ByteBuffer src) { | ||
| return commandAsyncImpl(src, src.remaining()); |
There was a problem hiding this comment.
commandAsyncImpl is used just once. Let's inline the code.
public CompletableFuture<DataStreamReply> commandAsync(ByteBuffer src) {
if (isClosed()) {
return JavaUtils.completeExceptionally(new AlreadyClosedException(
clientId + ": stream already closed, request=" + header));
}
return combineHeader(send(Type.STREAM_COMMAND, src, src.remaining(),
Collections.singleton(StandardWriteOption.FLUSH)));
}| private final CompletableFuture<DataStream> streamFuture; | ||
| private final AtomicReference<CompletableFuture<Long>> writeFuture; | ||
| private final RequestMetrics metrics; | ||
| private final AtomicReference<CompletableFuture<LocalResult>> writeFuture; |
There was a problem hiding this comment.
Let's rename it to resultFuture.
| CompletableFuture<LocalResult> write(ByteBuf buf, Iterable<WriteOption> options, | ||
| Executor executor) { |
| final ByteBuffer command = copyBuffer(request.slice()); | ||
| localResult = info.getLocal().command(command, request.getStreamOffset(), writeExecutor); | ||
| remoteWrites = info.applyToRemotes(out -> out.command( | ||
| copyBuffer(request.slice()), requestExecutor)); |
There was a problem hiding this comment.
duplicate() and reuse command, i.e.
final ByteBuffer command = copyBuffer(request.slice());
localResult = info.getLocal().command(command.duplicate(), request.getStreamOffset(), writeExecutor);
remoteWrites = info.applyToRemotes(out -> out.command(command, requestExecutor));| copy.put(buffer); | ||
| } | ||
| copy.flip(); | ||
| return copy; |
There was a problem hiding this comment.
Make it readonly
return copy.asReadOnlyBuffer();| return CompletableFuture.runAsync(() -> {}, e) | ||
| .thenCompose(ignored -> stream.onCommand(command, streamOffset)); |
There was a problem hiding this comment.
Using completedFuture(null) is more efficient since it doesn't have to run an empty task.
return CompletableFuture.completedFuture(null)
.thenComposeAsync(ignored -> stream.onCommand(command, streamOffset), e);There was a problem hiding this comment.
Thanks. I really learned something from this comment.
| reply = replyFuture.get(requestTimeout.getDuration(), requestTimeout.getUnit()); | ||
| } catch (Exception e) { | ||
| throw new CompletionException("Failed to get reply for bytesWritten=" + bytesWritten + ", " + request, e); | ||
| throw new CompletionException("Failed to get reply for " + localResult + ", " + request, e); |
There was a problem hiding this comment.
Add toString()
@Override
public String toString() {
return commandReply != null ? "commandReply:" + StringUtils.bytes2HexString(commandReply)
: "byteWritten:" + byteWritten;
}| } | ||
|
|
||
| @Override | ||
| public CompletableFuture<DataStreamReply> commandAsync(ByteBuffer src) { |
There was a problem hiding this comment.
What would happen on the client side if it issues two commands without data in between? As far as I can see, NettyClientReply, when it maps requests, considers only the stream offset and type, so the second command with the same offset wouldn't have a ReplyEntry.
There was a problem hiding this comment.
Very good call. It seems to me that the first command's ReplyEntry will be used for the second command and the second command's RequestEntry won't be in the map. Anyway it looks like a mess without a handling.
So I instead fail the commands if there is already one at the same stream offset. After all, we do not expect the caller issue multiple commands at the same stream offset for now.
szetszwo
left a comment
There was a problem hiding this comment.
@ss77892 , Good catch one the problem!
@amaliujia , thanks for the update! Please see the comment inlined.
| if (requestEntry.type == Type.STREAM_COMMAND) { | ||
| if (map.containsKey(requestEntry)) { | ||
| final IllegalStateException exception = new IllegalStateException( | ||
| "A STREAM_COMMAND is already pending for " + requestEntry | ||
| + " for " + clientInvocationId | ||
| + "; wait for the previous command reply before sending another"); | ||
| f.completeExceptionally(exception); | ||
| return null; | ||
| } | ||
| final ReplyEntry entry = new ReplyEntry(isClose, f); | ||
| map.put(requestEntry, entry); | ||
| return entry; | ||
| } | ||
| // ConcurrentHashMap.computeIfAbsent javadoc: the function is applied at most once per key. | ||
| return map.computeIfAbsent(requestEntry, r -> new ReplyEntry(isClose, f)); |
There was a problem hiding this comment.
ReplyEntry submitRequest(RequestEntry requestEntry, boolean isClose, CompletableFuture<DataStreamReply> f) {
LOG.debug("put {} to the map for {}", requestEntry, clientInvocationId);
final MemoizedSupplier<ReplyEntry> supplier = MemoizedSupplier.valueOf(() -> new ReplyEntry(isClose, f));
final ReplyEntry reply = map.computeIfAbsent(requestEntry, r -> supplier.get());
if (requestEntry.type == Type.STREAM_COMMAND && !supplier.isInitialized()) {
final IllegalStateException exception = new IllegalStateException(
"STREAM_COMMAND already exist: " + requestEntry + " for " + clientInvocationId);
f.completeExceptionally(exception);
return null;
}
return reply;
}- We should only make one call to the map as above; otherwise, it is not atomic. In this case, we may use MemoizedSupplier.
- Let's also make the exception message shorter; see below
There was a problem hiding this comment.
Done!
Thanks for the hint. The map is concurrent map so two accesses (one containsKey, one put) are not atomic and will cause concurrency issue.
Control plane seems not the right term. How about revising the title to below?
|
szetszwo
left a comment
There was a problem hiding this comment.
+1 the change looks good.
What changes were proposed in this pull request?
This PR is AI-assisted
Adds a first-class mid-stream command plane to Ratis data streams. Callers can send opaque command payloads during an active stream via DataStreamOutput.commandAsync(). The server delivers them to StateMachine.DataStream.onCommand() instead of writing bytes to the data channel. Commands are ordered with data writes, replicated to remote peers like STREAM_DATA, and do not advance the stream byte offset.
Example usage:
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/RATIS-2625
How was this patch tested?
Unit tests