You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@bookkeeper.apache.org by "YutSean (via GitHub)" <gi...@apache.org> on 2023/07/17 09:25:25 UTC
[GitHub] [bookkeeper] YutSean opened a new pull request, #4034: Added a sync function to run flush without commit #4033
YutSean opened a new pull request, #4034:
URL: https://github.com/apache/bookkeeper/pull/4034
Descriptions of the changes in this PR:
Added a sync method for AppendOnlyWriter
### Motivation
(Explain: why you're making that change, what is the problem you're trying to solve)
### Changes
The method has a parameter isforced. If true, the sync just works as force. If false, the sync method just invoke a flush method.
Master Issue: #<master-issue-number>
> ---
> In order to uphold a high standard for quality for code contributions, Apache BookKeeper runs various precommit
> checks for pull requests. A pull request can only be merged when it passes precommit checks.
>
> ---
> Be sure to do all of the following to help us incorporate your contribution
> quickly and easily:
>
> If this PR is a BookKeeper Proposal (BP):
>
> - [ ] Make sure the PR title is formatted like:
> `<BP-#>: Description of bookkeeper proposal`
> `e.g. BP-1: 64 bits ledger is support`
> - [ ] Attach the master issue link in the description of this PR.
> - [ ] Attach the google doc link if the BP is written in Google Doc.
>
> Otherwise:
>
> - [ ] Make sure the PR title is formatted like:
> `<Issue #>: Description of pull request`
> `e.g. Issue 123: Description ...`
> - [ ] Make sure tests pass via `mvn clean apache-rat:check install spotbugs:check`.
> - [ ] Replace `<Issue #>` in the title with the actual Issue number.
>
> ---
--
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: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] Reidddddd commented on a diff in pull request #4034: Issue #4033: Added a sync function to run flush without commit
Posted by "Reidddddd (via GitHub)" <gi...@apache.org>.
Reidddddd commented on code in PR #4034:
URL: https://github.com/apache/bookkeeper/pull/4034#discussion_r1266156504
##########
stream/distributedlog/core/src/main/java/org/apache/distributedlog/BKAsyncLogWriter.java:
##########
@@ -465,9 +465,24 @@ CompletableFuture<Long> flushAndCommit() {
if (null == writerFuture) {
return FutureUtils.value(getLastTxId());
}
- return writerFuture.thenCompose(writer -> writer.flushAndCommit());
+ return writerFuture.thenCompose(BKLogSegmentWriter::flushAndCommit);
Review Comment:
unrelated and unnecessary change... please keep as it is
--
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: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] YutSean commented on pull request #4034: Issue #4033: Added a sync function to run flush without commit
Posted by "YutSean (via GitHub)" <gi...@apache.org>.
YutSean commented on PR #4034:
URL: https://github.com/apache/bookkeeper/pull/4034#issuecomment-1639528416
Added a UT in TestAppendOnlyStreamWriter. The flush works as expected locally.
--
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: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] Reidddddd commented on pull request #4034: Issue #4033: Added a sync function to run flush without commit
Posted by "Reidddddd (via GitHub)" <gi...@apache.org>.
Reidddddd commented on PR #4034:
URL: https://github.com/apache/bookkeeper/pull/4034#issuecomment-1639299960
Better to provide a new test method or a new UT
--
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: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] eolivelli commented on pull request #4034: Issue #4033: Added a sync function to run flush without commit
Posted by "eolivelli (via GitHub)" <gi...@apache.org>.
eolivelli commented on PR #4034:
URL: https://github.com/apache/bookkeeper/pull/4034#issuecomment-1639666759
@diegosalvi FYI
--
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: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] YutSean commented on pull request #4034: Issue #4033: Added a sync function to run flush without commit
Posted by "YutSean (via GitHub)" <gi...@apache.org>.
YutSean commented on PR #4034:
URL: https://github.com/apache/bookkeeper/pull/4034#issuecomment-1645220987
Has added the motivation and details in the comments.
--
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: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] dlg99 commented on pull request #4034: Issue #4033: Added a sync function to run flush without commit
Posted by "dlg99 (via GitHub)" <gi...@apache.org>.
dlg99 commented on PR #4034:
URL: https://github.com/apache/bookkeeper/pull/4034#issuecomment-1638505265
Can you please provide more details for "(Explain: why you're making that change, what is the problem you're trying to solve)".
--
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: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] YutSean commented on a diff in pull request #4034: Issue #4033: Added a sync function to run flush without commit
Posted by "YutSean (via GitHub)" <gi...@apache.org>.
YutSean commented on code in PR #4034:
URL: https://github.com/apache/bookkeeper/pull/4034#discussion_r1266253948
##########
stream/distributedlog/core/src/main/java/org/apache/distributedlog/BKAsyncLogWriter.java:
##########
@@ -465,9 +465,24 @@ CompletableFuture<Long> flushAndCommit() {
if (null == writerFuture) {
return FutureUtils.value(getLastTxId());
}
- return writerFuture.thenCompose(writer -> writer.flushAndCommit());
+ return writerFuture.thenCompose(BKLogSegmentWriter::flushAndCommit);
Review Comment:
reverted the change
--
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: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] YutSean commented on pull request #4034: Issue #4033: Added a sync function to run flush without commit
Posted by "YutSean (via GitHub)" <gi...@apache.org>.
YutSean commented on PR #4034:
URL: https://github.com/apache/bookkeeper/pull/4034#issuecomment-1639152548
We are testing using Distributedlog as HBase WAL storage. For HBase, there is three different durability levels, async wal, sync wal, fsync wal. The features of these three kinds of wal are:
- async wal: the main thread only invokes append (add method of appendonlywriter) and returns.
- sync wal: the main thread returns after confirming that the data is already arrived at the storage service through network.
- fsync wal: the main thread returns after confirming that the data is already persistently stored.
But currently, the AppendOnlyWriter only provides a method force, which provides a stronger durability than fsync wal (the client will wait until the data is visible to other clients).
The new methods I added in this issue is just for the implementations of the above different kinds of durability.
I have tested the method in our test environment. It will boost the performance of the whole system.
--
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: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] Reidddddd commented on a diff in pull request #4034: Issue #4033: Added a sync function to run flush without commit
Posted by "Reidddddd (via GitHub)" <gi...@apache.org>.
Reidddddd commented on code in PR #4034:
URL: https://github.com/apache/bookkeeper/pull/4034#discussion_r1266154862
##########
stream/distributedlog/core/src/main/java/org/apache/distributedlog/AppendOnlyStreamWriter.java:
##########
@@ -68,6 +68,20 @@ public void force(boolean metadata) throws IOException {
}
}
+ public void flush(boolean waitForCompletion) throws IOException {
+ try {
+ if (waitForCompletion) {
+ FutureUtils.result(logWriter.flush());
+ } else {
+ logWriter.flush();
+ }
+ } catch (IOException ioe) {
+ throw ioe;
+ } catch (Exception ex) {
+ throw new UnexpectedException("unexpected exception in AppendOnlyStreamWriter.force", ex);
Review Comment:
Your comment is not changed... still **AppendOnlyStreamWriter.force**
--
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: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] YutSean commented on a diff in pull request #4034: Issue #4033: Added a sync function to run flush without commit
Posted by "YutSean (via GitHub)" <gi...@apache.org>.
YutSean commented on code in PR #4034:
URL: https://github.com/apache/bookkeeper/pull/4034#discussion_r1266254170
##########
stream/distributedlog/core/src/main/java/org/apache/distributedlog/AppendOnlyStreamWriter.java:
##########
@@ -68,6 +68,20 @@ public void force(boolean metadata) throws IOException {
}
}
+ public void flush(boolean waitForCompletion) throws IOException {
+ try {
+ if (waitForCompletion) {
+ FutureUtils.result(logWriter.flush());
+ } else {
+ logWriter.flush();
+ }
+ } catch (IOException ioe) {
+ throw ioe;
+ } catch (Exception ex) {
+ throw new UnexpectedException("unexpected exception in AppendOnlyStreamWriter.force", ex);
Review Comment:
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: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org