adoroszlai commented on code in PR #977:
URL: https://github.com/apache/ratis/pull/977#discussion_r1413151268
##########
ratis-client/src/main/java/org/apache/ratis/client/RaftClientConfigKeys.java:
##########
@@ -117,7 +117,7 @@ static void setFlushRequestCountMin(RaftProperties
properties, int flushRequestC
String FLUSH_REQUEST_BYTES_MIN_KEY = PREFIX + ".flush.request.bytes.min";
SizeInBytes FLUSH_REQUEST_BYTES_MIN_DEFAULT = SizeInBytes.ONE_MB;
static SizeInBytes flushRequestBytesMin(RaftProperties properties) {
- return getSizeInBytes(properties::getSizeInBytes,
FLUSH_REQUEST_COUNT_MIN_KEY,
+ return getSizeInBytes(properties::getSizeInBytes,
FLUSH_REQUEST_BYTES_MIN_KEY,
Review Comment:
Nice find.
However, it seems that `TestNettyDataStream*` has been passing only due to
this bug (the tests use `flush.request.count.min=4`, hence write requests >= 4
bytes were flushed, practically all of them). Now with this config bug fixed
the tests stall at client waiting for response.
https://github.com/apache/ratis/blob/214e8cd0a78ddde4d5eab605403cc226034ca0f3/ratis-test/src/test/java/org/apache/ratis/datastream/TestNettyDataStreamChainTopologyWithGrpcCluster.java#L35-L36
Changing to count=1 or bytes=4 allows the tests to pass again.
##########
ratis-netty/src/main/java/org/apache/ratis/netty/client/NettyClientStreamRpc.java:
##########
@@ -242,29 +249,44 @@ public String toString() {
}
}
- class OutstandingRequests {
+ static class OutstandingRequests {
private int count;
private long bytes;
- synchronized boolean write(DataStreamRequest request) {
- count++;
- bytes += request.getDataLength();
- final List<WriteOption> options = request.getWriteOptionList();
- final boolean isClose = options.contains(StandardWriteOption.CLOSE);
- final boolean isFlush = options.contains(StandardWriteOption.FLUSH);
- final boolean flush = shouldFlush(isClose || isFlush,
flushRequestCountMin, flushRequestBytesMin);
- LOG.debug("Stream{} outstanding: count={}, bytes={}, options={}, flush?
{}",
- request.getStreamId(), count, bytes, options, flush);
- return flush;
+ private boolean shouldFlush(List<WriteOption> options, int countMin,
SizeInBytes bytesMin) {
+ if (options.contains(StandardWriteOption.CLOSE)) {
+ // flush in order to send the CLOSE option.
+ return true;
+ } else if (bytes == 0 && count == 0) {
+ // nothing to flush (when bytes == 0 && count > 0, client may have
written empty packets for including options)
+ return false;
+ } else {
+ return count >= countMin
+ || bytes >= bytesMin.getSize()
+ || options.contains(StandardWriteOption.FLUSH);
Review Comment:
I've noticed that `FLUSH` is never really used.
If `options` contains `FLUSH` for a header message, we get the following
when encoding it:
```
NullPointerException
at
org.apache.ratis.proto.RaftProtos$DataStreamPacketHeaderProto$Builder.addOptions(RaftProtos.java:36161)
at
org.apache.ratis.netty.NettyDataStreamUtils.getDataStreamRequestHeaderProtoByteBuffer(NettyDataStreamUtils.java:60)
at
org.apache.ratis.netty.NettyDataStreamUtils.encodeDataStreamRequestHeader(NettyDataStreamUtils.java:92)
at
org.apache.ratis.netty.NettyDataStreamUtils.encodeDataStreamRequestByteBuffer(NettyDataStreamUtils.java:107)
```
due to the following proto conversion:
https://github.com/apache/ratis/blob/214e8cd0a78ddde4d5eab605403cc226034ca0f3/ratis-netty/src/main/java/org/apache/ratis/netty/NettyDataStreamUtils.java#L59-L62
since `FLUSH` is not defined in:
https://github.com/apache/ratis/blob/214e8cd0a78ddde4d5eab605403cc226034ca0f3/ratis-proto/src/main/proto/Raft.proto#L343-L346
If `FLUSH` needs to be sent to peers, we should add it in the proto.
On the other hand, if it needs to apply only locally, we should move it to a
separate `enum` which also implements `WriteOption`; and then only encode
options that are `StandardWriteOption`.
--
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]
