You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@bookkeeper.apache.org by GitBox <gi...@apache.org> on 2022/06/23 09:49:59 UTC
[GitHub] [bookkeeper] gaozhangmin opened a new pull request, #3353: Fix checkpointComplete invoked twice
gaozhangmin opened a new pull request, #3353:
URL: https://github.com/apache/bookkeeper/pull/3353
### Motivation
SyncThread flush would invoke `checkpointSource.checkpointComplete` twice. it's necessary.
### Changes
Change `org.apache.bookkeeper.bookie.LedgerStorage#flush` signature . add `doCheckpointComplete` parameter.
default value is true, only set to false in `org.apache.bookkeeper.bookie.SyncThread#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@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] StevenLuMT commented on pull request #3353: Fix checkpointComplete invoked twice in SyncThread
Posted by GitBox <gi...@apache.org>.
StevenLuMT commented on PR #3353:
URL: https://github.com/apache/bookkeeper/pull/3353#issuecomment-1225340888
fix old workflow,please see #3455 for detail
--
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 #3353: Fix checkpointComplete invoked twice in SyncThread
Posted by GitBox <gi...@apache.org>.
hangc0276 commented on code in PR #3353:
URL: https://github.com/apache/bookkeeper/pull/3353#discussion_r905729497
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/SingleDirectoryDbLedgerStorage.java:
##########
@@ -820,10 +820,12 @@ private void swapWriteCache() {
}
@Override
- public void flush() throws IOException {
+ public void flush(boolean doCheckpointComplete) throws IOException {
Checkpoint cp = checkpointSource.newCheckpoint();
checkpoint(cp);
- checkpointSource.checkpointComplete(cp, true);
+ if (doCheckpointComplete) {
Review Comment:
It is a little strange that we flush the WriteCache data into OS PageCache without writing the checkpoint `lastMark` into ledger directory.
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/SyncThread.java:
##########
@@ -133,7 +133,7 @@ public Future requestFlush() {
private void flush() {
Checkpoint checkpoint = checkpointSource.newCheckpoint();
Review Comment:
We create a checkpoint at line 134, and write the checkpoint lastMark position into ledger directory at line 152. There is a gap that the actually flushed lastMark position is less than write cache flushed position.
--
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@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] StevenLuMT commented on a diff in pull request #3353: Fix checkpointComplete invoked twice in SyncThread
Posted by GitBox <gi...@apache.org>.
StevenLuMT commented on code in PR #3353:
URL: https://github.com/apache/bookkeeper/pull/3353#discussion_r929157553
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerStorage.java:
##########
@@ -180,8 +180,9 @@ void cancelWaitForLastAddConfirmedUpdate(long ledgerId,
* Flushes all data in the storage. Once this is called,
* add data written to the LedgerStorage up until this point
* has been persisted to perminant storage
+ * @param doCheckpointComplete
*/
- void flush() throws IOException;
+ void flush(boolean doCheckpointComplete) throws IOException;
Review Comment:
How do we charge what this value(doCheckpointComplete) is passed in?
--
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@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] gaozhangmin closed pull request #3353: Fix checkpointComplete invoked twice in SyncThread
Posted by "gaozhangmin (via GitHub)" <gi...@apache.org>.
gaozhangmin closed pull request #3353: Fix checkpointComplete invoked twice in SyncThread
URL: https://github.com/apache/bookkeeper/pull/3353
--
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] gaozhangmin commented on a diff in pull request #3353: Fix checkpointComplete invoked twice in SyncThread
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on code in PR #3353:
URL: https://github.com/apache/bookkeeper/pull/3353#discussion_r908217953
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/SyncThread.java:
##########
@@ -133,7 +133,7 @@ public Future requestFlush() {
private void flush() {
Checkpoint checkpoint = checkpointSource.newCheckpoint();
Review Comment:
Any suggestion? We just remove `checkpointSource.checkpointComplete(checkpoint, false);` here?
But the parameter `disableCheckpoint ` won't take effect any more.
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/SyncThread.java:
##########
@@ -133,7 +133,7 @@ public Future requestFlush() {
private void flush() {
Checkpoint checkpoint = checkpointSource.newCheckpoint();
Review Comment:
Any suggestion? We just remove `checkpointSource.checkpointComplete(checkpoint, false);` here?
But the parameter `disableCheckpoint ` won't take effect any more. @hangc0276
--
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@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] gaozhangmin commented on a diff in pull request #3353: Fix checkpointComplete invoked twice in SyncThread
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on code in PR #3353:
URL: https://github.com/apache/bookkeeper/pull/3353#discussion_r908211261
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/SingleDirectoryDbLedgerStorage.java:
##########
@@ -820,10 +820,12 @@ private void swapWriteCache() {
}
@Override
- public void flush() throws IOException {
+ public void flush(boolean doCheckpointComplete) throws IOException {
Checkpoint cp = checkpointSource.newCheckpoint();
checkpoint(cp);
- checkpointSource.checkpointComplete(cp, true);
+ if (doCheckpointComplete) {
Review Comment:
In SyncThread, We can disableCheckpoint, just return without executing `checkpointSource.checkpointComplete(checkpoint, false);` I don't think this is strange.
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/SingleDirectoryDbLedgerStorage.java:
##########
@@ -820,10 +820,12 @@ private void swapWriteCache() {
}
@Override
- public void flush() throws IOException {
+ public void flush(boolean doCheckpointComplete) throws IOException {
Checkpoint cp = checkpointSource.newCheckpoint();
checkpoint(cp);
- checkpointSource.checkpointComplete(cp, true);
+ if (doCheckpointComplete) {
Review Comment:
In SyncThread, We can disableCheckpoint, just return without executing `checkpointSource.checkpointComplete(checkpoint, false);` I don't think this is strange. @hangc0276
--
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@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org