You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@bookkeeper.apache.org by "merlimat (via GitHub)" <gi...@apache.org> on 2023/03/01 21:25:58 UTC
[GitHub] [bookkeeper] merlimat opened a new pull request, #3830: Simplified the logic for ForceWriteThread after we introduced queue.drainTo()
merlimat opened a new pull request, #3830:
URL: https://github.com/apache/bookkeeper/pull/3830
### Motivation
In #3545 we have switched the `ForceWriteThread` to take advantage o `BlockingQueue.drainTo()` method for reducing contention, though the core logic of the force-write was not touched at the time.
The logic of force-write is quite complicated because it tries to group multiple force-write requests in the queue by sending a new marker and grouping them when the marker is received. This also leads to a bit of lag when there are many requests coming in and the IO is stressed, as we're waiting a bit more before issuing the fsync.
Instead, with the `drainTo()` approach we can greatly simplify the logic and maintaining a strict fsync grouping:
1. drain all the force-write-requests available in the queue into a local array list
2. perform the fsync
3. update the journal log mark to the position of the last fw request
4. trigger send-responses for all the requests
5. go back to read from the queue
--
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] merlimat commented on a diff in pull request #3830: Simplified the logic for ForceWriteThread after we introduced queue.drainTo()
Posted by "merlimat (via GitHub)" <gi...@apache.org>.
merlimat commented on code in PR #3830:
URL: https://github.com/apache/bookkeeper/pull/3830#discussion_r1123655830
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Journal.java:
##########
@@ -534,11 +484,26 @@ public void run() {
requestsCount = 1;
}
+ journalStats.getForceWriteQueueSize().addCount(-requestsCount);
+
+ // Sync and mark the journal up to the position of the last entry in the batch
+ ForceWriteRequest lastRequest = localRequests.get(requestsCount - 1);
+ syncJournal(lastRequest);
Review Comment:
Actually, you're correct. We need to ensure all the files are closed before the response is triggered. Fixed 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: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] hangc0276 commented on pull request #3830: Simplified the logic for ForceWriteThread after we introduced queue.drainTo()
Posted by "hangc0276 (via GitHub)" <gi...@apache.org>.
hangc0276 commented on PR #3830:
URL: https://github.com/apache/bookkeeper/pull/3830#issuecomment-1453355782
Please rebase the master after https://github.com/apache/bookkeeper/pull/3836 is merged to trigger the CI.
--
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] merlimat commented on a diff in pull request #3830: Simplified the logic for ForceWriteThread after we introduced queue.drainTo()
Posted by "merlimat (via GitHub)" <gi...@apache.org>.
merlimat commented on code in PR #3830:
URL: https://github.com/apache/bookkeeper/pull/3830#discussion_r1124673748
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Journal.java:
##########
@@ -534,11 +484,26 @@ public void run() {
requestsCount = 1;
}
+ journalStats.getForceWriteQueueSize().addCount(-requestsCount);
+
+ // Sync and mark the journal up to the position of the last entry in the batch
+ ForceWriteRequest lastRequest = localRequests.get(requestsCount - 1);
+ syncJournal(lastRequest);
Review Comment:
All good points. I'll fix 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: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] merlimat commented on a diff in pull request #3830: Simplified the logic for ForceWriteThread after we introduced queue.drainTo()
Posted by "merlimat (via GitHub)" <gi...@apache.org>.
merlimat commented on code in PR #3830:
URL: https://github.com/apache/bookkeeper/pull/3830#discussion_r1123325633
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Journal.java:
##########
@@ -534,11 +484,26 @@ public void run() {
requestsCount = 1;
}
+ journalStats.getForceWriteQueueSize().addCount(-requestsCount);
+
+ // Sync and mark the journal up to the position of the last entry in the batch
+ ForceWriteRequest lastRequest = localRequests.get(requestsCount - 1);
+ syncJournal(lastRequest);
Review Comment:
Yes, it's possible though the Journal thread would have already closed the previous file, so we don't need to either fsync or close 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: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] hangc0276 commented on a diff in pull request #3830: Simplified the logic for ForceWriteThread after we introduced queue.drainTo()
Posted by "hangc0276 (via GitHub)" <gi...@apache.org>.
hangc0276 commented on code in PR #3830:
URL: https://github.com/apache/bookkeeper/pull/3830#discussion_r1123232237
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Journal.java:
##########
@@ -534,11 +484,26 @@ public void run() {
requestsCount = 1;
}
+ journalStats.getForceWriteQueueSize().addCount(-requestsCount);
+
+ // Sync and mark the journal up to the position of the last entry in the batch
+ ForceWriteRequest lastRequest = localRequests.get(requestsCount - 1);
+ syncJournal(lastRequest);
Review Comment:
+1
If the localRequests queue contains multiple journal files and we only sync the lastRequest's journal file, other journal files will skip sync.
--
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] hangc0276 commented on a diff in pull request #3830: Simplified the logic for ForceWriteThread after we introduced queue.drainTo()
Posted by "hangc0276 (via GitHub)" <gi...@apache.org>.
hangc0276 commented on code in PR #3830:
URL: https://github.com/apache/bookkeeper/pull/3830#discussion_r1124308286
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Journal.java:
##########
@@ -534,11 +484,26 @@ public void run() {
requestsCount = 1;
}
+ journalStats.getForceWriteQueueSize().addCount(-requestsCount);
+
+ // Sync and mark the journal up to the position of the last entry in the batch
+ ForceWriteRequest lastRequest = localRequests.get(requestsCount - 1);
+ syncJournal(lastRequest);
Review Comment:
+1
It has two bugs:
- Those non-last journal files in the batch won't be removed from OS PageCache
- That non-last journal files channel in the batch will just call `close` instead of `fore(false)`, which can't ensure the data is flushed to the disk.
--
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] hangc0276 merged pull request #3830: Simplified the logic for ForceWriteThread after we introduced queue.drainTo()
Posted by "hangc0276 (via GitHub)" <gi...@apache.org>.
hangc0276 merged PR #3830:
URL: https://github.com/apache/bookkeeper/pull/3830
--
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] merlimat commented on a diff in pull request #3830: Simplified the logic for ForceWriteThread after we introduced queue.drainTo()
Posted by "merlimat (via GitHub)" <gi...@apache.org>.
merlimat commented on code in PR #3830:
URL: https://github.com/apache/bookkeeper/pull/3830#discussion_r1124676997
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Journal.java:
##########
@@ -534,11 +484,26 @@ public void run() {
requestsCount = 1;
}
+ journalStats.getForceWriteQueueSize().addCount(-requestsCount);
+
+ // Sync and mark the journal up to the position of the last entry in the batch
+ ForceWriteRequest lastRequest = localRequests.get(requestsCount - 1);
+ syncJournal(lastRequest);
Review Comment:
I was actually 100% sure that close() implied an fsync, but that's not really 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: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] codecov-commenter commented on pull request #3830: Simplified the logic for ForceWriteThread after we introduced queue.drainTo()
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #3830:
URL: https://github.com/apache/bookkeeper/pull/3830#issuecomment-1454818839
# [Codecov](https://codecov.io/gh/apache/bookkeeper/pull/3830?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#3830](https://codecov.io/gh/apache/bookkeeper/pull/3830?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (1879a97) into [master](https://codecov.io/gh/apache/bookkeeper/commit/b4112dfdbf29c16da89d914d44faaa8c821723a8?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (b4112df) will **decrease** coverage by `47.25%`.
> The diff coverage is `68.96%`.
```diff
@@ Coverage Diff @@
## master #3830 +/- ##
=============================================
- Coverage 68.21% 20.96% -47.25%
+ Complexity 6761 2018 -4743
=============================================
Files 473 473
Lines 40950 40889 -61
Branches 5240 5229 -11
=============================================
- Hits 27935 8574 -19361
- Misses 10762 31049 +20287
+ Partials 2253 1266 -987
```
| Flag | Coverage Δ | |
|---|---|---|
| bookie | `?` | |
| client | `?` | |
| remaining | `?` | |
| replication | `?` | |
| tls | `20.96% <68.96%> (+<0.01%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/bookkeeper/pull/3830?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...g/apache/bookkeeper/bookie/stats/JournalStats.java](https://codecov.io/gh/apache/bookkeeper/pull/3830?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Ym9va2tlZXBlci1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2Jvb2trZWVwZXIvYm9va2llL3N0YXRzL0pvdXJuYWxTdGF0cy5qYXZh) | `81.63% <ø> (-7.85%)` | :arrow_down: |
| [...ain/java/org/apache/bookkeeper/bookie/Journal.java](https://codecov.io/gh/apache/bookkeeper/pull/3830?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Ym9va2tlZXBlci1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2Jvb2trZWVwZXIvYm9va2llL0pvdXJuYWwuamF2YQ==) | `65.87% <68.96%> (-14.90%)` | :arrow_down: |
| [.../java/org/apache/bookkeeper/util/SubTreeCache.java](https://codecov.io/gh/apache/bookkeeper/pull/3830?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Ym9va2tlZXBlci1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2Jvb2trZWVwZXIvdXRpbC9TdWJUcmVlQ2FjaGUuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...java/org/apache/bookkeeper/proto/BookieClient.java](https://codecov.io/gh/apache/bookkeeper/pull/3830?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Ym9va2tlZXBlci1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2Jvb2trZWVwZXIvcHJvdG8vQm9va2llQ2xpZW50LmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...a/org/apache/bookkeeper/client/api/BookKeeper.java](https://codecov.io/gh/apache/bookkeeper/pull/3830?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Ym9va2tlZXBlci1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2Jvb2trZWVwZXIvY2xpZW50L2FwaS9Cb29rS2VlcGVyLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...a/org/apache/bookkeeper/client/api/ReadHandle.java](https://codecov.io/gh/apache/bookkeeper/pull/3830?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Ym9va2tlZXBlci1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2Jvb2trZWVwZXIvY2xpZW50L2FwaS9SZWFkSGFuZGxlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...org/apache/bookkeeper/bookie/ReadOnlyFileInfo.java](https://codecov.io/gh/apache/bookkeeper/pull/3830?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Ym9va2tlZXBlci1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2Jvb2trZWVwZXIvYm9va2llL1JlYWRPbmx5RmlsZUluZm8uamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...org/apache/bookkeeper/bookie/datainteg/Events.java](https://codecov.io/gh/apache/bookkeeper/pull/3830?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Ym9va2tlZXBlci1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2Jvb2trZWVwZXIvYm9va2llL2RhdGFpbnRlZy9FdmVudHMuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...rg/apache/bookkeeper/client/api/CreateBuilder.java](https://codecov.io/gh/apache/bookkeeper/pull/3830?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Ym9va2tlZXBlci1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2Jvb2trZWVwZXIvY2xpZW50L2FwaS9DcmVhdGVCdWlsZGVyLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...rg/apache/bookkeeper/metastore/MetastoreTable.java](https://codecov.io/gh/apache/bookkeeper/pull/3830?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Ym9va2tlZXBlci1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2Jvb2trZWVwZXIvbWV0YXN0b3JlL01ldGFzdG9yZVRhYmxlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [370 more](https://codecov.io/gh/apache/bookkeeper/pull/3830?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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] zymap commented on a diff in pull request #3830: Simplified the logic for ForceWriteThread after we introduced queue.drainTo()
Posted by "zymap (via GitHub)" <gi...@apache.org>.
zymap commented on code in PR #3830:
URL: https://github.com/apache/bookkeeper/pull/3830#discussion_r1124280282
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Journal.java:
##########
@@ -534,11 +484,26 @@ public void run() {
requestsCount = 1;
}
+ journalStats.getForceWriteQueueSize().addCount(-requestsCount);
+
+ // Sync and mark the journal up to the position of the last entry in the batch
+ ForceWriteRequest lastRequest = localRequests.get(requestsCount - 1);
+ syncJournal(lastRequest);
Review Comment:
Original logic will execute forceWrite before close, which will run bestEffortRemoveFromPageCache. If there have two different journal files in the batch, we only force write for the last file, do we need to do the force write for the another journal file? Does the `close` will do that?
--
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] zymap commented on a diff in pull request #3830: Simplified the logic for ForceWriteThread after we introduced queue.drainTo()
Posted by "zymap (via GitHub)" <gi...@apache.org>.
zymap commented on code in PR #3830:
URL: https://github.com/apache/bookkeeper/pull/3830#discussion_r1122536754
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Journal.java:
##########
@@ -534,11 +484,26 @@ public void run() {
requestsCount = 1;
}
+ journalStats.getForceWriteQueueSize().addCount(-requestsCount);
+
+ // Sync and mark the journal up to the position of the last entry in the batch
+ ForceWriteRequest lastRequest = localRequests.get(requestsCount - 1);
+ syncJournal(lastRequest);
Review Comment:
Is it possible that there have two log files in the batch?
--
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