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/07/28 07:12:25 UTC
[GitHub] [ratis] ChenSammi opened a new pull request, #699: RATIS-1644. Improve unsafe flush to safe async flush.
ChenSammi opened a new pull request, #699:
URL: https://github.com/apache/ratis/pull/699
https://issues.apache.org/jira/browse/RATIS-1644
--
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] ChenSammi commented on pull request #699: RATIS-1644. Provide a safe async flush.
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on PR #699:
URL: https://github.com/apache/ratis/pull/699#issuecomment-1204865305
IMHO, the current updateIncreasingly is less strict than updateToMax. updateToMax will atomically check if the new value is greater than the old value before update the index while updateIncreasingly doesn the check after the value change in a way which has no atomic guarantee.
This async flush just put the flush aside, the number of flush is not reduced. In a heavy workload cluster, every flush took up to seconds to finish. My goal is first make it async, then reduce the actual flush count by reverse order executing, flush tasks with high commitIndex got executed first.
--
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] ChenSammi commented on pull request #699: RATIS-1644. Improve unsafe flush to safe async flush.
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on PR #699:
URL: https://github.com/apache/ratis/pull/699#issuecomment-1201915853
@szetszwo , Right. currently when raft.server.log.statemachine.data.sync is true, state machine data will be waited to be synced before ratis moves on to write the corresponding raft log and flush. So we do not handle any state machine related case in flushIfNecessary(), right? Let me know if I misunderstand something.
--
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 diff in pull request #699: RATIS-1644. Provide a safe async flush.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on code in PR #699:
URL: https://github.com/apache/ratis/pull/699#discussion_r935773079
##########
ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/SegmentedRaftLogWorker.java:
##########
@@ -217,7 +218,8 @@ synchronized void updateIndex(long i) {
final int bufferSize = RaftServerConfigKeys.Log.writeBufferSize(properties).getSizeInt();
this.writeBuffer = ByteBuffer.allocateDirect(bufferSize);
this.unsafeFlush = RaftServerConfigKeys.Log.unsafeFlushEnabled(properties);
- this.flushExecutor = !unsafeFlush? null
+ this.asyncFlush = RaftServerConfigKeys.Log.asyncFlushEnabled(properties);
Review Comment:
We should throw an exception or print a warning when both asyncFlush and unsafeFlush are true.
--
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] ChenSammi commented on pull request #699: RATIS-1644. Provide a safe async flush.
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on PR #699:
URL: https://github.com/apache/ratis/pull/699#issuecomment-1204743602
> @ChenSammi , thanks for the update! Please see https://issues.apache.org/jira/secure/attachment/13047652/699_review.patch for the review suggestions.
@szetszwo , the difference is it sticks to call updateIncreasingly on flushIndex in your proposal while updateToMax is called currently. Is there any known side effect of using updateToMax? I'm thinking of using PriorityBlockingList in flushExecutor in another PR to improve the performance by executing the raft flush with high commitIndex in priority, and remove all lower commitIndex flush tasks, so that we will have less actual file flush. In this case, flushIndex will not be update continuously.
--
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] ChenSammi commented on pull request #699: RATIS-1644. Improve unsafe flush to safe async flush.
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on PR #699:
URL: https://github.com/apache/ratis/pull/699#issuecomment-1200938843
> @ChenSammi , thanks for working on this. Async flush is a very good idea! Some thoughts
>
> * We should support unsafe flush since the existing unsafe flush still has a better performance than async flush. @SincereXIA , do you need unsafe flush if we have the async flush provided here?
>
> * Async flush needs to honor stateMachineDataPolicy; otherwise, it is unsafe.
@szetszwo The stateMachineDataPolicy is controlled by raft.server.log.statemachine.data.sync. When it's true, state machine will be flushed before it reaches raft log write and flush. When it's false, the current sync flush is unsafe too.
Since we have defined the raft.server.log.statemachine.data.sync to control the state machine data flush, we can just focus on the write log flush with .async-flush.enabled property. Otherwise, maybe we should merge this two property into one to guarantee a safe flush of both state machine data and log data with sync and async two modes. What do you think?
--
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] ChenSammi commented on pull request #699: RATIS-1644. Improve unsafe flush to safe async flush.
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on PR #699:
URL: https://github.com/apache/ratis/pull/699#issuecomment-1200650000
Thanks @szetszwo and @captainzmc for the feedback.
--
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 diff in pull request #699: RATIS-1644. Provide a safe async flush.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on code in PR #699:
URL: https://github.com/apache/ratis/pull/699#discussion_r943039944
##########
ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/BufferedWriteChannel.java:
##########
@@ -91,23 +98,26 @@ void flush() throws IOException {
}
}
- CompletableFuture<Void> asyncFlush(ExecutorService executor) throws IOException {
+ CompletableFuture<Long> asyncFlush(ExecutorService executor, Long writtenIndex) throws IOException {
Review Comment:
Let's don't pass `writtenIndex`, which is not used anywhere except for returning back in the future. It makes the method complicated.
##########
ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/BufferedWriteChannel.java:
##########
@@ -133,13 +143,19 @@ boolean isOpen() {
}
@Override
+ @SuppressFBWarnings("RV_RETURN_VALUE_IGNORED_NO_SIDE_EFFECT")
public void close() throws IOException {
if (!isOpen()) {
return;
}
try {
fileChannel.truncate(fileChannel.position());
+ callback.get().get();
Review Comment:
We should wait for the flush `callback` first and then truncate. BTW, let's rename `callback` to `flushFuture` or `flushFutureSupplier`? Otherwise, it is hard to know why does `callback` mean.
##########
ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/BufferedWriteChannel.java:
##########
@@ -91,23 +98,26 @@ void flush() throws IOException {
}
}
- CompletableFuture<Void> asyncFlush(ExecutorService executor) throws IOException {
+ CompletableFuture<Long> asyncFlush(ExecutorService executor, Long writtenIndex) throws IOException {
flushBuffer();
if (forced) {
return CompletableFuture.completedFuture(null);
}
- final CompletableFuture<Void> f = CompletableFuture.supplyAsync(this::fileChannelForce, executor);
+ final CompletableFuture<Long> f = new CompletableFuture();
+ executor.execute(() -> fileChannelForce(writtenIndex, f));
forced = true;
+
return f;
}
- private Void fileChannelForce() {
+ private void fileChannelForce(Long lastWrittenIndex, CompletableFuture future) {
Review Comment:
Similarly, let's don't pass `lastWrittenIndex` and `future`. A standard way is to use ` CompletableFuture.supplyAsync(..)`.
##########
ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/BufferedWriteChannel.java:
##########
@@ -45,16 +50,18 @@ static BufferedWriteChannel open(File file, boolean append, ByteBuffer buffer) t
fc.truncate(0);
}
Preconditions.assertSame(fc.size(), fc.position(), "fc.position");
- return new BufferedWriteChannel(fc, buffer);
+ return new BufferedWriteChannel(fc, buffer, closeCallback);
}
private final FileChannel fileChannel;
private final ByteBuffer writeBuffer;
private boolean forced = true;
+ private final Supplier<CompletableFuture> callback;
Review Comment:
Please specify the type parameter. Otherwise, there is a "Raw use of parameterized class 'CompletableFuture'" warning.
--
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 #699: RATIS-1644. Provide a safe async flush.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on PR #699:
URL: https://github.com/apache/ratis/pull/699#issuecomment-1205503642
> ... the current updateIncreasingly is less strict than updateToMax. updateToMax will atomically check if the new value is greater than the old value before update the index while updateIncreasingly doesn the check after the value change in a way which has no atomic guarantee.
Yes, updateIncreasingly does the check after the value change and it will throw an exception if the check failed. The check is an assertion in order to make sure that the condition always holds. If it fails, it is a bug in the code.
Since updateIncreasingly only allows the new value >= the old value while updateToMax allows any value, we say updateIncreasingly more strict than updateToMax in this sense.
--
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 diff in pull request #699: RATIS-1644. Provide a safe async flush.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on code in PR #699:
URL: https://github.com/apache/ratis/pull/699#discussion_r947023694
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/ServerProtoUtils.java:
##########
@@ -171,4 +171,34 @@ static boolean isSuccess(InstallSnapshotResult result) {
return false;
}
}
+
+ public static String convertToString(InstallSnapshotRequestProto request) {
Review Comment:
Oops, I forgot that we already had `ServerStringUtils.toInstallSnapshotRequestString(..)`. Filed https://issues.apache.org/jira/browse/RATIS-1676
--
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] ChenSammi commented on pull request #699: RATIS-1644. Provide a safe async flush.
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on PR #699:
URL: https://github.com/apache/ratis/pull/699#issuecomment-1217362385
Thanks @szetszwo for the code review.
--
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 #699: RATIS-1644. Provide a safe async flush.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on PR #699:
URL: https://github.com/apache/ratis/pull/699#issuecomment-1212345305
@ChenSammi , there is a test failure which can be reproduced. Could you take a look?
https://github.com/apache/ratis/runs/7793088310?check_suite_focus=true#step:5:625
--
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 #699: RATIS-1644. Improve unsafe flush to safe async flush.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on PR #699:
URL: https://github.com/apache/ratis/pull/699#issuecomment-1201430256
@ChenSammi, yes, when raft.server.log.statemachine.data.sync is false, we don't have to do anything.
When both raft.server.log.statemachine.data.sync and the new Async flush are true, we should wait for both operations to complete before updating the index.
--
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 #699: RATIS-1644. Provide a safe async flush.
Posted by GitBox <gi...@apache.org>.
szetszwo merged PR #699:
URL: https://github.com/apache/ratis/pull/699
--
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 #699: RATIS-1644. Improve unsafe flush to safe async flush.
Posted by GitBox <gi...@apache.org>.
captainzmc commented on PR #699:
URL: https://github.com/apache/ratis/pull/699#issuecomment-1198841033
> * We should support unsafe flush since the existing unsafe flush still has a better performance than async flush. @SincereXIA , do you need unsafe flush if we have the async flush provided here?
Thanks @szetszwo @ChenSammi. Yes we'd better keep unsafe flush because of the better performance. We can add a separate safe async flush 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] ChenSammi commented on pull request #699: RATIS-1644. Provide a safe async flush.
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on PR #699:
URL: https://github.com/apache/ratis/pull/699#issuecomment-1207920009
@szetszwo thanks for the explain. It makes sense. Please take a look of the new PR at your convenient time, 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] ChenSammi commented on pull request #699: RATIS-1644. Provide a safe async flush.
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on PR #699:
URL: https://github.com/apache/ratis/pull/699#issuecomment-1216625923
TestRaftSnapshotWithNetty failure seems irrelevant. Anyway, it's fixed.
--
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 #699: RATIS-1644. Provide a safe async flush.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on PR #699:
URL: https://github.com/apache/ratis/pull/699#issuecomment-1204816228
@ChenSammi , updateIncreasingly is more strict than updateToMax so that it is preferred. For flush index, it should be updated increasingly. Do you agree?
--
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 diff in pull request #699: RATIS-1644. Provide a safe async flush.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on code in PR #699:
URL: https://github.com/apache/ratis/pull/699#discussion_r935781998
##########
ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/SegmentedRaftLogWorker.java:
##########
@@ -375,22 +377,50 @@ private void flushIfNecessary() throws IOException {
if (unsafeFlush) {
// unsafe-flush: call updateFlushedIndexIncreasingly() without waiting the underlying FileChannel.force(..).
unsafeFlushOutStream();
+ updateFlushedIndexIncreasingly();
+ } else if (asyncFlush) {
+ asyncFlushOutStream();
Review Comment:
We should combine stateMachine flush future `f`.
```
+++ b/ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/SegmentedRaftLogWorker.java
@@ -379,7 +379,7 @@ class SegmentedRaftLogWorker {
unsafeFlushOutStream();
updateFlushedIndexIncreasingly();
} else if (asyncFlush) {
- asyncFlushOutStream();
+ asyncFlushOutStream(f);
} else {
flushOutStream();
if (!stateMachineDataPolicy.isSync()) {
@@ -398,9 +398,11 @@ class SegmentedRaftLogWorker {
out.asyncFlush(flushExecutor, null).whenComplete((v, e) -> logSyncTimerContext.stop());
}
- private void asyncFlushOutStream() throws IOException {
+ private void asyncFlushOutStream(CompletableFuture<Void> stateMachineFlush) throws IOException {
final Timer.Context logSyncTimerContext = raftLogSyncTimer.time();
- out.asyncFlush(flushExecutor, lastWrittenIndex).whenComplete((v, e) -> {
+ out.asyncFlush(flushExecutor, lastWrittenIndex)
+ .thenCombine(stateMachineFlush, (async, stateMachine) -> async)
+ .whenComplete((v, e) -> {
updateFlushedIndexIncreasingly(v);
logSyncTimerContext.stop();
});
```
##########
ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/SegmentedRaftLogWorker.java:
##########
@@ -217,7 +218,8 @@ synchronized void updateIndex(long i) {
final int bufferSize = RaftServerConfigKeys.Log.writeBufferSize(properties).getSizeInt();
this.writeBuffer = ByteBuffer.allocateDirect(bufferSize);
this.unsafeFlush = RaftServerConfigKeys.Log.unsafeFlushEnabled(properties);
- this.flushExecutor = !unsafeFlush? null
+ this.asyncFlush = RaftServerConfigKeys.Log.asyncFlushEnabled(properties);
Review Comment:
We should throw an exception when both asyncFlush and unsafeFlush are true.
--
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