You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ratis.apache.org by GitBox <gi...@apache.org> on 2022/03/03 09:36:08 UTC
[GitHub] [ratis] SincereXIA opened a new pull request #616: RATIS-1545. RaftLogOutputStream support async flush.
SincereXIA opened a new pull request #616:
URL: https://github.com/apache/ratis/pull/616
## What changes were proposed in this pull request?
We previously tried to reduce disk IO by introducing a minimum interval between flushes, the relevant pr is here: https://github.com/apache/ratis/pull/611
However, after subsequent tests, we found that the raft performance has degraded, because `flushIfNecessary()` function does more operations than just flushing the stream.
Since we enforced the minimum time for each flush, the raft log to can't be committed in flush intervals, which reduces the performance of raft.
So we want to separate `out.flush()` from `flushIfNecessary()` and execute it asynchronously, so that the process of writing to disk will not block the operation of other threads
## What is the link to the Apache JIRA
https://issues.apache.org/jira/projects/RATIS/issues/RATIS-1545
## How was this patch tested?
We use the ozone freon ockg tool to detect the effect of the changes.
test command:
```
bin/ozone freon ockg --volume=volume --bucket=defaultbucket --thread=10 --number-of-tests=300 --size=`expr 32 \* 1048576` --prefix=stream
```
We tested the speed of writing 32MB and 128MB objects after enabe async flush:
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] SincereXIA commented on a change in pull request #616: RATIS-1545. RaftLogOutputStream support async flush.
Posted by GitBox <gi...@apache.org>.
SincereXIA commented on a change in pull request #616:
URL: https://github.com/apache/ratis/pull/616#discussion_r820013916
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/SegmentedRaftLogWorker.java
##########
@@ -355,7 +356,7 @@ private boolean shouldFlush() {
} else if (pendingFlushNum >= forceSyncNum) {
return true;
}
- return pendingFlushNum > 0 && queue.isEmpty() && lastFlush.elapsedTime().compareTo(flushIntervalMin) > 0;
Review comment:
> @SincereXIA , We already have raft.server.log.flush.interval.min . Do you mean adding another flush interval?
In my test, I found that add flush interval in this way has bug, It will greatly reduce the submission speed of Raft log. So I had to remove it.
So affter use this pr, I can't increase the flush interval.
I figured out a way to increase the flush interval: https://github.com/SincereXIA/ratis/commit/118b2e5ec5339b2fb105c29e738c066644112d59 It add a sleep in async flush call task, and it will not block the main thread. But after I increased the flush interval by this method, the performance did not improve. So I didn't add this commit to this pr. I haven't been able to think of a better way to do it.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on a change in pull request #616: RATIS-1545. RaftLogOutputStream support async flush.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on a change in pull request #616:
URL: https://github.com/apache/ratis/pull/616#discussion_r820032942
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/SegmentedRaftLogWorker.java
##########
@@ -371,21 +372,32 @@ private void flushIfNecessary() throws IOException {
if (stateMachineDataPolicy.isSync()) {
stateMachineDataPolicy.getFromFuture(f, () -> this + "-flushStateMachineData");
}
- final Timer.Context logSyncTimerContext = raftLogSyncTimer.time();
flushBatchSize = (int)(lastWrittenIndex - flushIndex.get());
- out.flush();
- logSyncTimerContext.stop();
+ if (flushExecutorWorkQueue.isEmpty()) {
Review comment:
Good point!
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/SegmentedRaftLogWorker.java
##########
@@ -371,21 +372,32 @@ private void flushIfNecessary() throws IOException {
if (stateMachineDataPolicy.isSync()) {
stateMachineDataPolicy.getFromFuture(f, () -> this + "-flushStateMachineData");
}
- final Timer.Context logSyncTimerContext = raftLogSyncTimer.time();
flushBatchSize = (int)(lastWrittenIndex - flushIndex.get());
- out.flush();
- logSyncTimerContext.stop();
+ if (flushExecutorWorkQueue.isEmpty()) {
+ CompletableFuture.supplyAsync(this::flushOutStream, flushExecutor);
Review comment:
Since it is async, it may call updateFlushedIndexIncreasingly() before flush actually has completed. It is incorrect.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] SincereXIA commented on a change in pull request #616: WIP: RATIS-1545. RaftLogOutputStream support async flush.
Posted by GitBox <gi...@apache.org>.
SincereXIA commented on a change in pull request #616:
URL: https://github.com/apache/ratis/pull/616#discussion_r819381124
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/SegmentedRaftLogWorker.java
##########
@@ -371,21 +372,32 @@ private void flushIfNecessary() throws IOException {
if (stateMachineDataPolicy.isSync()) {
stateMachineDataPolicy.getFromFuture(f, () -> this + "-flushStateMachineData");
}
- final Timer.Context logSyncTimerContext = raftLogSyncTimer.time();
flushBatchSize = (int)(lastWrittenIndex - flushIndex.get());
- out.flush();
- logSyncTimerContext.stop();
+ if (flushExecutorWorkQueue.isEmpty()) {
Review comment:
I think adding another flush work isn't necessary if there are already flush tasks in the queue that haven't been executed yet.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] kaijchen commented on pull request #616: RATIS-1545. RaftLogOutputStream support async flush.
Posted by GitBox <gi...@apache.org>.
kaijchen commented on pull request #616:
URL: https://github.com/apache/ratis/pull/616#issuecomment-1061496165
> * Why async flush with current flushIntervalMin causing a bug?
`flushIntervalMin` is actually max time to wait (for a full batch) before flush.
It will slow down the service significantly if it was set to something like `3s`.
Each log can take up to 3s to be committed if the QPS is very low.
It's not a bug, but this value needs to be set carefully.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] SincereXIA commented on a change in pull request #616: RATIS-1545. RaftLogOutputStream support async flush.
Posted by GitBox <gi...@apache.org>.
SincereXIA commented on a change in pull request #616:
URL: https://github.com/apache/ratis/pull/616#discussion_r820013916
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/SegmentedRaftLogWorker.java
##########
@@ -355,7 +356,7 @@ private boolean shouldFlush() {
} else if (pendingFlushNum >= forceSyncNum) {
return true;
}
- return pendingFlushNum > 0 && queue.isEmpty() && lastFlush.elapsedTime().compareTo(flushIntervalMin) > 0;
Review comment:
> @SincereXIA , We already have raft.server.log.flush.interval.min . Do you mean adding another flush interval?
In my test, I found that add flush interval in this way has bug, It will greatly reduce the submission speed of Raft log. So I had to remove it.
For now, I can't increase the refresh interval.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] SincereXIA commented on a change in pull request #616: RATIS-1545. RaftLogOutputStream support async flush.
Posted by GitBox <gi...@apache.org>.
SincereXIA commented on a change in pull request #616:
URL: https://github.com/apache/ratis/pull/616#discussion_r819381124
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/SegmentedRaftLogWorker.java
##########
@@ -371,21 +372,32 @@ private void flushIfNecessary() throws IOException {
if (stateMachineDataPolicy.isSync()) {
stateMachineDataPolicy.getFromFuture(f, () -> this + "-flushStateMachineData");
}
- final Timer.Context logSyncTimerContext = raftLogSyncTimer.time();
flushBatchSize = (int)(lastWrittenIndex - flushIndex.get());
- out.flush();
- logSyncTimerContext.stop();
+ if (flushExecutorWorkQueue.isEmpty()) {
Review comment:
I think adding another flush work isn't necessary, if there already has flush tasks in the queue that haven't been executed yet.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] kaijchen edited a comment on pull request #616: RATIS-1545. RaftLogOutputStream support async flush.
Posted by GitBox <gi...@apache.org>.
kaijchen edited a comment on pull request #616:
URL: https://github.com/apache/ratis/pull/616#issuecomment-1061496165
> * Why async flush with current flushIntervalMin causing a bug?
`flushIntervalMin` is actually max time to wait (for a full batch) before flush.
It will introduce latency while improve throughput.
If it is set to something like `3s`, each log can take `up to 3s` to be committed.
It's not a bug, but this value needs to be set carefully.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] SincereXIA edited a comment on pull request #616: RATIS-1545. RaftLogOutputStream support async flush.
Posted by GitBox <gi...@apache.org>.
SincereXIA edited a comment on pull request #616:
URL: https://github.com/apache/ratis/pull/616#issuecomment-1061541975
Hi @szetszwo, Thanks for your opinion. I made a small change and tested your patch, and the result is here:
When unsafe flush + interval 3s is enabled, the average write performance of Ozone streaming is improved by 25%
> Why async flush is slower then the single-thread flush?
In the first version I implemented, I pulled `out.flush()` off the main thread and executed it every fixed time interval in a separate thread.
So `out.flush()` was executed less than async flush. I guess that is the reason why async flush is a little slower.
> Why async flush with current flushIntervalMin causing a bug?
For this question, @kaijchen is right. Exactly, it's not a bug. In the last pr, I didn't consider that extending the refresh interval will cause the log submission time to be extended. The performance did not meet our expectations.
In the current version, I use flushIntervalMin to control the flush interval of unsafe flush, which will not prolong the time that the log is finally committed.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] SincereXIA commented on a change in pull request #616: RATIS-1545. RaftLogOutputStream support async flush.
Posted by GitBox <gi...@apache.org>.
SincereXIA commented on a change in pull request #616:
URL: https://github.com/apache/ratis/pull/616#discussion_r821450785
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/SegmentedRaftLogOutputStream.java
##########
@@ -67,7 +67,9 @@ public SegmentedRaftLogOutputStream(File file, boolean append, long segmentMaxSi
// write header
preallocateIfNecessary(SegmentedRaftLogFormat.getHeaderLength());
SegmentedRaftLogFormat.applyHeaderTo(CheckedConsumer.asCheckedFunction(out::write));
- out.flush();
+ synchronized (out) {
+ out.flush();
Review comment:
We found errors like this during testing:
```
2022-03-08 11:15:23,856 [97cb3f28-f298-4ed3-93c7-3be44bfe8b7b@group-892C7CF01088-SegmentedRaftLogWorker] ERROR org.apache.ratis.server.raftlog.segmented.SegmentedRaftLogWorker: 97cb3f28-f298-4ed3-93c7-3be44bfe8b7b@group-892C7CF01088-SegmentedRaftLogWorker hit exception
java.lang.IllegalArgumentException
at java.nio.Buffer.position(Buffer.java:244)
at java.nio.DirectByteBuffer.put(DirectByteBuffer.java:377)
at org.apache.ratis.server.raftlog.segmented.BufferedWriteChannel.write(BufferedWriteChannel.java:61)
at org.apache.ratis.server.raftlog.segmented.SegmentedRaftLogOutputStream.write(SegmentedRaftLogOutputStream.java:100)
at org.apache.ratis.server.raftlog.segmented.SegmentedRaftLogWorker$WriteLog.execute(SegmentedRaftLogWorker.java:610)
at org.apache.ratis.server.raftlog.segmented.SegmentedRaftLogWorker.run(SegmentedRaftLogWorker.java:404)
at java.lang.Thread.run(Thread.java:748)
2022-03-08 11:15:23,858 [97cb3f28-f298-4ed3-93c7-3be44bfe8b7b@group-892C7CF01088-SegmentedRaftLogWorker] INFO org.apache.ratis.server.RaftServer$Division: 97cb3f28-f298-4ed3-93c7-3be44bfe8b7b@group-892C7CF01088: shutdown
```
I think the cause of this error is that class `BufferedWriteChannel` is not thread safe, but an asynchronous flush may operate on this object at the same time as the main thread.
```
/**
* Provides a buffering layer in front of a FileChannel for writing.
*
* This class is NOT threadsafe.
*/
class BufferedWriteChannel implements Closeable {
````
After we make it synchronized, the error no longer occurs.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on pull request #616: RATIS-1545. RaftLogOutputStream support async flush.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on pull request #616:
URL: https://github.com/apache/ratis/pull/616#issuecomment-1059244053
> I tried increasing the interval between async flushes like this: https://github.com/SincereXIA/ratis/commit/118b2e5ec5339b2fb105c29e738c066644112d59
It seems the sleep in https://github.com/SincereXIA/ratis/commit/118b2e5ec5339b2fb105c29e738c066644112d59 is the problem.
Could you try https://issues.apache.org/jira/secure/attachment/13040724/616_asyncFlush.patch (applying to master) ?
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] SincereXIA edited a comment on pull request #616: RATIS-1545. RaftLogOutputStream support async flush.
Posted by GitBox <gi...@apache.org>.
SincereXIA edited a comment on pull request #616:
URL: https://github.com/apache/ratis/pull/616#issuecomment-1059143121
> @SincereXIA , could you try also async flush with 3s flush interval? It seems that ExecutorService and single thread are supposed to have the same performance. Thanks.
Hi @szetszwo , I tried increasing the interval between async flushes like this: https://github.com/SincereXIA/ratis/commit/118b2e5ec5339b2fb105c29e738c066644112d59
and I repeated the test, but I couldn't find a difference in performance by use 3s flush interval. The average write time is almost the same as the last column in the table.
I think maybe the time it takes to create the async task is causing the slight drop in performance.
By the way, is it a good idea to add a configurable flush interval? If yes, I can add it.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] kaijchen edited a comment on pull request #616: RATIS-1545. RaftLogOutputStream support async flush.
Posted by GitBox <gi...@apache.org>.
kaijchen edited a comment on pull request #616:
URL: https://github.com/apache/ratis/pull/616#issuecomment-1061496165
> * Why async flush with current flushIntervalMin causing a bug?
`flushIntervalMin` is actually max time to wait (for a full batch) before flush.
It will introduce latency while improve throughput.
If it is set to something like `3s`, each log can take up to `3s` to be committed.
It's not a bug, but this value needs to be set carefully.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo merged pull request #616: RATIS-1545. RaftLogOutputStream support async flush.
Posted by GitBox <gi...@apache.org>.
szetszwo merged pull request #616:
URL: https://github.com/apache/ratis/pull/616
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on pull request #616: RATIS-1545. RaftLogOutputStream support async flush.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on pull request #616:
URL: https://github.com/apache/ratis/pull/616#issuecomment-1061477782
@SincereXIA , if we want to support unsafe-flush (i.e. increase flush index without waiting flush to complete), we don't need to use FlushWorker anymore since it is for safe-flush. We should answer the following two questions:
- Why async flush is slower then the single-thread flush?
- Why async flush with current flushIntervalMin causing a bug?
Could you test https://issues.apache.org/jira/secure/attachment/13040828/616_unsafeFlush.patch ?
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] SincereXIA commented on pull request #616: RATIS-1545. RaftLogOutputStream support async flush.
Posted by GitBox <gi...@apache.org>.
SincereXIA commented on pull request #616:
URL: https://github.com/apache/ratis/pull/616#issuecomment-1061667024
> @SincereXIA , do you think that the first version is better? We should use it if it is the case.
@szetszwo , I think this version now is better, In the first version, there will always be a thread that repeatedly does meaningless flush. And the performance of the previous version is tested without locking. It is not safe. After add lock at the `SegmentedRaftLogOutputStream`, the performance of the previous version may be not better than the current version.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on pull request #616: RATIS-1545. RaftLogOutputStream support async flush.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on pull request #616:
URL: https://github.com/apache/ratis/pull/616#issuecomment-1061720501
@SincereXIA , thanks for the update.
> We found errors like this during testing: ...
Since FileChannel is thread safe, let's call FileChannel.force(..) asynchronously so that we don't have to synchronize all the output stream calls; see https://issues.apache.org/jira/secure/attachment/13040845/616_asyncFileChannelForce.patch
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] SincereXIA commented on pull request #616: RATIS-1545. RaftLogOutputStream support async flush.
Posted by GitBox <gi...@apache.org>.
SincereXIA commented on pull request #616:
URL: https://github.com/apache/ratis/pull/616#issuecomment-1059143121
> @SincereXIA , could you try also async flush with 3s flush interval? It seems that ExecutorService and single thread are supposed to have the same performance. Thanks.
Hi @szetszwo , I tried increasing the interval between async flushes like this: https://github.com/SincereXIA/ratis/commit/118b2e5ec5339b2fb105c29e738c066644112d59
and I repeated the test, but I couldn't find a difference in performance by use 3s flush interval. The average write time is almost the same as the last column in the table.
I think maybe the time it takes to create the async task is causing the slight drop in performance.
By the way, is it a good idea to add a configurable refresh interval? If yes, I can add it.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on a change in pull request #616: RATIS-1545. RaftLogOutputStream support async flush.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on a change in pull request #616:
URL: https://github.com/apache/ratis/pull/616#discussion_r820051767
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/SegmentedRaftLogWorker.java
##########
@@ -217,8 +216,9 @@ synchronized void updateIndex(long i) {
final int bufferSize = RaftServerConfigKeys.Log.writeBufferSize(properties).getSizeInt();
this.writeBuffer = ByteBuffer.allocateDirect(bufferSize);
- this.lastFlush = Timestamp.currentTime();
- this.flushIntervalMin = RaftServerConfigKeys.Log.flushIntervalMin(properties);
+ this.flushExecutorWorkQueue = new ArrayBlockingQueue<>(1);
+ this.flushExecutor = new ThreadPoolExecutor(1, 1, 0L, TimeUnit.MILLISECONDS,
Review comment:
Use a 60s keepAliveTime as below -- the zero keepAliveTime probably is the reason that ExecutorService being slower than single Thread.
```
this.flushExecutor = new ThreadPoolExecutor(1, 1, 60L, TimeUnit.SECONDS,
this.flushExecutorWorkQueue, ConcurrentUtils.newThreadFactory(name + "-flush"));
```
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on pull request #616: RATIS-1545. RaftLogOutputStream support async flush.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on pull request #616:
URL: https://github.com/apache/ratis/pull/616#issuecomment-1060452546
@SincereXIA , thanks a lot for testing it.
> ... But I I want to know if we can add a switch ...
Yes, that's a good idea, especially for the groups with 3 or more nodes.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] SincereXIA commented on pull request #616: RATIS-1545. RaftLogOutputStream support async flush.
Posted by GitBox <gi...@apache.org>.
SincereXIA commented on pull request #616:
URL: https://github.com/apache/ratis/pull/616#issuecomment-1060288959
> @SincereXIA , we need to make sure that the flushIndex is updated only after flush has completed. Here is the idea https://issues.apache.org/jira/secure/attachment/13040750/616_FlushWorker.patch . Please see if you agree with the approach. Thanks.
Hi @szetszwo, Thank you. We tested this patch, but we found that calling `updateFlushedIndexIncreasingly()` after the flush cause
ratis performance is almost the same as synchronous flush. The RaftLog can't be committed until we call `updateFlushedIndexIncreasingly()` . So, may work will still be blocked by the out.flush() call.
Committing the raft log before the flush is fully completed may indeed be a security issue. But I I want to know if we can add a switch to allow users to choose whether they can accept sacrificing a certain amount of security in exchange for higher performance. Thanks.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] SincereXIA commented on a change in pull request #616: RATIS-1545. RaftLogOutputStream support async flush.
Posted by GitBox <gi...@apache.org>.
SincereXIA commented on a change in pull request #616:
URL: https://github.com/apache/ratis/pull/616#discussion_r819217805
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/SegmentedRaftLogWorker.java
##########
@@ -349,13 +354,25 @@ private void run() {
}
}
+ private void checkAndFlush() {
+ while (running) {
+ try {
+ outStreamFlushIfNecessary();
+ flushIntervalMin.sleep();
+ } catch (Exception e) {
+ LOG.error("{} hit exception", Thread.currentThread().getName(), e);
+ Optional.ofNullable(server).ifPresent(RaftServer.Division::close);
+ }
+ }
+ }
+
private boolean shouldFlush() {
if (out == null) {
return false;
} else if (pendingFlushNum >= forceSyncNum) {
return true;
}
- return pendingFlushNum > 0 && queue.isEmpty() && lastFlush.elapsedTime().compareTo(flushIntervalMin) > 0;
+ return pendingFlushNum > 0 && queue.isEmpty();
Review comment:
Sure, I changed it
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on pull request #616: RATIS-1545. RaftLogOutputStream support async flush.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on pull request #616:
URL: https://github.com/apache/ratis/pull/616#issuecomment-1059031028
@SincereXIA , could you try also async flush with 3s flush interval? It seems that ExecutorService and single thread are supposed to have the same performance. Thanks.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on pull request #616: RATIS-1545. RaftLogOutputStream support async flush.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on pull request #616:
URL: https://github.com/apache/ratis/pull/616#issuecomment-1061650030
> In the first version I implemented, I pulled out.flush() off the main thread and executed it every fixed time interval in a separate thread.
@SincereXIA , do you think that the first version is better? We should use it if it is the case.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] SincereXIA commented on a change in pull request #616: RATIS-1545. RaftLogOutputStream support async flush.
Posted by GitBox <gi...@apache.org>.
SincereXIA commented on a change in pull request #616:
URL: https://github.com/apache/ratis/pull/616#discussion_r819218886
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/SegmentedRaftLogWorker.java
##########
@@ -144,6 +144,7 @@ synchronized void updateIndex(long i) {
private final WriteLogTasks writeTasks = new WriteLogTasks();
private volatile boolean running = true;
private final Thread workerThread;
+ private final Thread outStreamFlushThread;
Review comment:
okay
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] SincereXIA edited a comment on pull request #616: RATIS-1545. RaftLogOutputStream support async flush.
Posted by GitBox <gi...@apache.org>.
SincereXIA edited a comment on pull request #616:
URL: https://github.com/apache/ratis/pull/616#issuecomment-1060288959
> @SincereXIA , we need to make sure that the flushIndex is updated only after flush has completed. Here is the idea https://issues.apache.org/jira/secure/attachment/13040750/616_FlushWorker.patch . Please see if you agree with the approach. Thanks.
Hi @szetszwo, Thank you. We tested this patch, but we found that calling `updateFlushedIndexIncreasingly()` after the flush cause
ratis performance is almost the same as synchronous flush. The RaftLog can't be committed until we call `updateFlushedIndexIncreasingly()` . So, may work will still be blocked by the out.flush() call.
Committing the raft log before the flush is fully completed may indeed be a security issue. But I I want to know if we can add a switch to allow users to choose whether they can accept sacrificing a little security in exchange for higher performance. Thanks.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] SincereXIA commented on pull request #616: RATIS-1545. RaftLogOutputStream support async flush.
Posted by GitBox <gi...@apache.org>.
SincereXIA commented on pull request #616:
URL: https://github.com/apache/ratis/pull/616#issuecomment-1059641731
> > I tried increasing the interval between async flushes like this: [SincereXIA@118b2e5](https://github.com/SincereXIA/ratis/commit/118b2e5ec5339b2fb105c29e738c066644112d59)
>
> @SincereXIA , It seems the sleep in [SincereXIA@118b2e5](https://github.com/SincereXIA/ratis/commit/118b2e5ec5339b2fb105c29e738c066644112d59) is the problem.
>
> Could you try https://issues.apache.org/jira/secure/attachment/13040724/616_asyncFlush.patch (applying to master) ?
Hi @szetszwo , This commit: https://github.com/SincereXIA/ratis/commit/118b2e5ec5339b2fb105c29e738c066644112d59 is only in my test branch, and is not included in this pr. So I think the changes in this pr is same as this patch: https://issues.apache.org/jira/secure/attachment/13040724/616_asyncFlush.patch , and the test result is here:
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] kaijchen commented on pull request #616: RATIS-1545. RaftLogOutputStream support async flush.
Posted by GitBox <gi...@apache.org>.
kaijchen commented on pull request #616:
URL: https://github.com/apache/ratis/pull/616#issuecomment-1061709022
@szetszwo @SincereXIA, Actually, there **is** a bug in current master.
We need check the flush interval against first log pending to flush, not last flush.
The following patch demonstrates the fix on current master (a4ffad20dfcc3f61d723e835db9aac075286deb3).
```java
diff --git a/ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/SegmentedRaftLogWorker.java b/ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/SegmentedRaftLogWorker.java
index 9e5eb7e1..b6e10a4c 100644
--- a/ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/SegmentedRaftLogWorker.java
+++ b/ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/SegmentedRaftLogWorker.java
@@ -177,6 +177,7 @@ class SegmentedRaftLogWorker {
private int flushBatchSize;
private Timestamp lastFlush;
+ private Timestamp firstArrival;
private final TimeDuration flushIntervalMin;
private final StateMachineDataPolicy stateMachineDataPolicy;
@@ -218,6 +219,7 @@ class SegmentedRaftLogWorker {
final int bufferSize = RaftServerConfigKeys.Log.writeBufferSize(properties).getSizeInt();
this.writeBuffer = ByteBuffer.allocateDirect(bufferSize);
this.lastFlush = Timestamp.currentTime();
+ this.firstArrival = Timestamp.currentTime();
this.flushIntervalMin = RaftServerConfigKeys.Log.flushIntervalMin(properties);
}
@@ -355,7 +357,10 @@ class SegmentedRaftLogWorker {
} else if (pendingFlushNum >= forceSyncNum) {
return true;
}
- return pendingFlushNum > 0 && queue.isEmpty() && lastFlush.elapsedTime().compareTo(flushIntervalMin) > 0;
+ if (lastFlush.compareTo(firstArrival) >= 0) {
+ firstArrival = Timestamp.currentTime();
+ }
+ return pendingFlushNum > 0 && queue.isEmpty() && firstArrival.elapsedTime().compareTo(flushIntervalMin) > 0;
}
@SuppressFBWarnings("NP_NULL_PARAM_DEREF")
--
2.27.0
```
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] SincereXIA commented on pull request #616: RATIS-1545. RaftLogOutputStream support async flush.
Posted by GitBox <gi...@apache.org>.
SincereXIA commented on pull request #616:
URL: https://github.com/apache/ratis/pull/616#issuecomment-1062524876
> Since FileChannel is thread safe, let's call FileChannel.force(..) asynchronously so that we don't have to synchronize all the output stream calls; see https://issues.apache.org/jira/secure/attachment/13040845/616_asyncFileChannelForce.patch
@szetszwo , Thanks for your opinion. We tested the performance of this patch:
Direct call `FileChannel.force(..)` does have better performance than synchronize all the output stream calls.
But we find a issue, sleep at here is useless. Because this will block the main thread and will not reduce the total number of flushes. So I removed `flushIntervalMin` for now.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] SincereXIA commented on pull request #616: RATIS-1545. RaftLogOutputStream support async flush.
Posted by GitBox <gi...@apache.org>.
SincereXIA commented on pull request #616:
URL: https://github.com/apache/ratis/pull/616#issuecomment-1061541975
Hi @szetszwo, Thanks for your opinion. I made a small change and tested your patch, and the result is here:
When unsafe flush + interval 3s is enabled, the average write performance of Ozone streaming is improved by 25%
> Why async flush is slower then the single-thread flush?
In the first version I implemented, I pulled out.flush() off the main thread and executed it every fixed time interval in a separate thread.
So out.flush() was executed less than async flush. I guess that is the reason why async flush is a little slower.
> Why async flush with current flushIntervalMin causing a bug?
For this question, @kaijchen is right. Exactly, it's not a bug. In the last pr, I didn't consider that extending the refresh interval will cause the log submission time to be extended. The performance did not meet our expectations.
In the current version, I use flushIntervalMin to control the flush interval of unsafe flush, which will not prolong the time that the log is finally committed.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo edited a comment on pull request #616: RATIS-1545. RaftLogOutputStream support async flush.
Posted by GitBox <gi...@apache.org>.
szetszwo edited a comment on pull request #616:
URL: https://github.com/apache/ratis/pull/616#issuecomment-1059244053
> I tried increasing the interval between async flushes like this: https://github.com/SincereXIA/ratis/commit/118b2e5ec5339b2fb105c29e738c066644112d59
@SincereXIA , It seems the sleep in https://github.com/SincereXIA/ratis/commit/118b2e5ec5339b2fb105c29e738c066644112d59 is the problem.
Could you try https://issues.apache.org/jira/secure/attachment/13040724/616_asyncFlush.patch (applying to master) ?
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on a change in pull request #616: RATIS-1545. RaftLogOutputStream support async flush.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on a change in pull request #616:
URL: https://github.com/apache/ratis/pull/616#discussion_r820051767
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/SegmentedRaftLogWorker.java
##########
@@ -217,8 +216,9 @@ synchronized void updateIndex(long i) {
final int bufferSize = RaftServerConfigKeys.Log.writeBufferSize(properties).getSizeInt();
this.writeBuffer = ByteBuffer.allocateDirect(bufferSize);
- this.lastFlush = Timestamp.currentTime();
- this.flushIntervalMin = RaftServerConfigKeys.Log.flushIntervalMin(properties);
+ this.flushExecutorWorkQueue = new ArrayBlockingQueue<>(1);
+ this.flushExecutor = new ThreadPoolExecutor(1, 1, 0L, TimeUnit.MILLISECONDS,
Review comment:
Use a 60s keepAliveTime as below -- the zero keepAliveTime probably is the reason that ExecutorService being slower than single Thread.
```
this.flushExecutor = new ThreadPoolExecutor(1, 1, 60L, TimeUnit.SECONDS,
this.flushExecutorWorkQueue, ConcurrentUtils.newThreadFactory(name + "-flush"));
```
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] SincereXIA commented on pull request #616: RATIS-1545. RaftLogOutputStream support async flush.
Posted by GitBox <gi...@apache.org>.
SincereXIA commented on pull request #616:
URL: https://github.com/apache/ratis/pull/616#issuecomment-1057856722
Hi @szetszwo, this pr is related to https://github.com/apache/ratis/pull/611, can you help review this?
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on a change in pull request #616: WIP: RATIS-1545. RaftLogOutputStream support async flush.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on a change in pull request #616:
URL: https://github.com/apache/ratis/pull/616#discussion_r819251195
##########
File path: ratis-server-api/src/main/java/org/apache/ratis/server/RaftServerConfigKeys.java
##########
@@ -348,15 +348,14 @@ static void setForceSyncNum(RaftProperties properties, int forceSyncNum) {
setInt(properties::setInt, FORCE_SYNC_NUM_KEY, forceSyncNum);
}
-
- String FLUSH_INTERVAL_MIN_KEY = PREFIX + ".flush.interval.min";
- TimeDuration FLUSH_INTERVAL_MIN_DEFAULT = TimeDuration.ZERO;
- static TimeDuration flushIntervalMin(RaftProperties properties) {
- return getTimeDuration(properties.getTimeDuration(FLUSH_INTERVAL_MIN_DEFAULT.getUnit()),
- FLUSH_INTERVAL_MIN_KEY, FLUSH_INTERVAL_MIN_DEFAULT, getDefaultLog());
+ String ASYNC_FLUSH_ENABLED_KEY = PREFIX + ".async.flush.enabled";
Review comment:
@SincereXIA , if the performance can be improved by async, let's just change the code to async and don't need this conf.
I guess you are testing the change. Will wait for your results. Thanks a lot!
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on pull request #616: RATIS-1545. RaftLogOutputStream support async flush.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on pull request #616:
URL: https://github.com/apache/ratis/pull/616#issuecomment-1059888027
@SincereXIA , we need to make sure that the flushIndex is updated only after flush has completed. Here is the idea https://issues.apache.org/jira/secure/attachment/13040750/616_FlushWorker.patch . Please see if you agree with the approach. Thanks.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] captainzmc commented on pull request #616: RATIS-1545. RaftLogOutputStream support async flush.
Posted by GitBox <gi...@apache.org>.
captainzmc commented on pull request #616:
URL: https://github.com/apache/ratis/pull/616#issuecomment-1060774230
> @SincereXIA , thanks a lot for testing it.
>
> > ... But I I want to know if we can add a switch ...
>
> Yes, that's a good idea, especially for the groups with 3 or more nodes.
+1 For make this configurable. After all, in most cases, we have a three replication. Async flush not only optimizes streaming, but also improves async write performance by more than 35% by we tested.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on a change in pull request #616: RATIS-1545. RaftLogOutputStream support async flush.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on a change in pull request #616:
URL: https://github.com/apache/ratis/pull/616#discussion_r818574232
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/SegmentedRaftLogWorker.java
##########
@@ -144,6 +144,7 @@ synchronized void updateIndex(long i) {
private final WriteLogTasks writeTasks = new WriteLogTasks();
private volatile boolean running = true;
private final Thread workerThread;
+ private final Thread outStreamFlushThread;
Review comment:
Let's use an ExecutorService.
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/SegmentedRaftLogWorker.java
##########
@@ -176,7 +177,7 @@ synchronized void updateIndex(long i) {
private final RaftServer.Division server;
private int flushBatchSize;
- private Timestamp lastFlush;
+ private Timestamp outStreamLastFlush;
Review comment:
State machine and out stream should either flush together or not flush at all. We should not try to separate them. So let's don't rename this field.
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/SegmentedRaftLogWorker.java
##########
@@ -349,13 +354,25 @@ private void run() {
}
}
+ private void checkAndFlush() {
+ while (running) {
+ try {
+ outStreamFlushIfNecessary();
+ flushIntervalMin.sleep();
+ } catch (Exception e) {
+ LOG.error("{} hit exception", Thread.currentThread().getName(), e);
+ Optional.ofNullable(server).ifPresent(RaftServer.Division::close);
+ }
+ }
+ }
+
private boolean shouldFlush() {
if (out == null) {
return false;
} else if (pendingFlushNum >= forceSyncNum) {
return true;
}
- return pendingFlushNum > 0 && queue.isEmpty() && lastFlush.elapsedTime().compareTo(flushIntervalMin) > 0;
+ return pendingFlushNum > 0 && queue.isEmpty();
Review comment:
Let's change flush to async but not the min time logic.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] SincereXIA commented on pull request #616: WIP: RATIS-1545. RaftLogOutputStream support async flush.
Posted by GitBox <gi...@apache.org>.
SincereXIA commented on pull request #616:
URL: https://github.com/apache/ratis/pull/616#issuecomment-1058988882
@szetszwo Thanks for your attention to this issue, Our tests show that using the thread pool scheme to asynchronous flush has obvious performance improvement over synchronous flush, but a little worse performance than using a separate thread：
Do you think this is ok?
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] SincereXIA commented on a change in pull request #616: RATIS-1545. RaftLogOutputStream support async flush.
Posted by GitBox <gi...@apache.org>.
SincereXIA commented on a change in pull request #616:
URL: https://github.com/apache/ratis/pull/616#discussion_r819219140
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/SegmentedRaftLogWorker.java
##########
@@ -176,7 +177,7 @@ synchronized void updateIndex(long i) {
private final RaftServer.Division server;
private int flushBatchSize;
- private Timestamp lastFlush;
+ private Timestamp outStreamLastFlush;
Review comment:
The lastFlush Timestamp is useless now, I removed it
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on pull request #616: RATIS-1545. RaftLogOutputStream support async flush.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on pull request #616:
URL: https://github.com/apache/ratis/pull/616#issuecomment-1059247558
> By the way, is it a good idea to add a configurable flush interval? If yes, I can add it.
@SincereXIA , We already have raft.server.log.flush.interval.min . Do you mean adding another flush interval?
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] SincereXIA commented on pull request #616: RATIS-1545. RaftLogOutputStream support async flush.
Posted by GitBox <gi...@apache.org>.
SincereXIA commented on pull request #616:
URL: https://github.com/apache/ratis/pull/616#issuecomment-1062559448
> +1 the change looks good.
>
> @SincereXIA , let's merge this version? Or, would you like to test it more?
@szetszwo, I have tested it, it looks ok. I think this version is ready for merge. Thank you.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org