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 2021/12/16 04:05:14 UTC
[GitHub] [bookkeeper] kezhuw opened a new pull request #2944: Fix wrong ledger id parse radix for index relocation file in IndexPersistenceMgr
kezhuw opened a new pull request #2944:
URL: https://github.com/apache/bookkeeper/pull/2944
Descriptions of the changes in this PR:
### Motivation
1. `FileInfo` was marked as deleted after moving to new location.
2. `IndexPersistenceMgr` fails to start after crashing index file movement.
### Changes
1. Add test to assert index file relocation
2. Don't mark FileInfo as deleted after successful index file relocation
3. Fix wrong ledger id parse radix for index relocation file in IndexPersistenceMgr
Master Issue: None
--
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] dlg99 commented on a change in pull request #2944: Fix wrong ledger id parse radix for index relocation file in IndexPersistenceMgr
Posted by GitBox <gi...@apache.org>.
dlg99 commented on a change in pull request #2944:
URL: https://github.com/apache/bookkeeper/pull/2944#discussion_r823218336
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/FileInfo.java
##########
@@ -549,6 +549,7 @@ public synchronized void moveToNewLocation(File newFile, long size) throws IOExc
}
fc = new RandomAccessFile(newFile, mode).getChannel();
lf = newFile;
+ deleted = false;
Review comment:
@nicoloboschi ping
--
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] kezhuw commented on pull request #2944: Fix wrong ledger id parse radix for index relocation file in IndexPersistenceMgr
Posted by GitBox <gi...@apache.org>.
kezhuw commented on pull request #2944:
URL: https://github.com/apache/bookkeeper/pull/2944#issuecomment-1085386830
@dlg99 @nicoloboschi Could you please take another look ? I have introduced `FileInfo.deleteFileDirectly` as spying point to avoid touching `deleted` in success path.
--
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] nicoloboschi commented on a change in pull request #2944: Fix wrong ledger id parse radix for index relocation file in IndexPersistenceMgr
Posted by GitBox <gi...@apache.org>.
nicoloboschi commented on a change in pull request #2944:
URL: https://github.com/apache/bookkeeper/pull/2944#discussion_r770360041
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/FileInfo.java
##########
@@ -549,6 +549,7 @@ public synchronized void moveToNewLocation(File newFile, long size) throws IOExc
}
fc = new RandomAccessFile(newFile, mode).getChannel();
lf = newFile;
+ deleted = false;
Review comment:
it'd be better to have a method `delete(boolean markAsDeleted)` and passing false in this method
in your new code we still have few lines where the `deleted` flag is equals to `true` and it can lead in undefined behaviour
--
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] kezhuw commented on pull request #2944: Fix wrong ledger id parse radix for index relocation file in IndexPersistenceMgr
Posted by GitBox <gi...@apache.org>.
kezhuw commented on pull request #2944:
URL: https://github.com/apache/bookkeeper/pull/2944#issuecomment-1085346939
rerun failure checks
--
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] kezhuw commented on a change in pull request #2944: Fix wrong ledger id parse radix for index relocation file in IndexPersistenceMgr
Posted by GitBox <gi...@apache.org>.
kezhuw commented on a change in pull request #2944:
URL: https://github.com/apache/bookkeeper/pull/2944#discussion_r823266442
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/FileInfo.java
##########
@@ -549,6 +549,7 @@ public synchronized void moveToNewLocation(File newFile, long size) throws IOExc
}
fc = new RandomAccessFile(newFile, mode).getChannel();
lf = newFile;
+ deleted = false;
Review comment:
> `moveToNewLocation` is not expected the be in deleted state at all during the rename
It is only correct in ideal situation. Could this operation happen cross filesystems or even devices ? I think this method make no assumption about this. From my side, `delete` in `moveToNewLocation` is more like a conservative operation than solely `Files.delete()`(which throw exception on deletion failure). After failure of `FileInfo.delete`, `FileInfo` is unusable as `FileInfo.fc` is closed and `FileInfo.lf` is not guaranteed to exist.
I have no preference over approaches:
* Current: `FileInfo.delete()` and throws exception on failure.
* Alternative: `FileInfo.lf.delete()` and mark `deleted` and throws exception on failure.
I think they are identical.
--
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] dlg99 commented on pull request #2944: Fix wrong ledger id parse radix for index relocation file in IndexPersistenceMgr
Posted by GitBox <gi...@apache.org>.
dlg99 commented on pull request #2944:
URL: https://github.com/apache/bookkeeper/pull/2944#issuecomment-1057325873
Close to reopen (and trigger rebase/CI run)
--
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] dlg99 closed pull request #2944: Fix wrong ledger id parse radix for index relocation file in IndexPersistenceMgr
Posted by GitBox <gi...@apache.org>.
dlg99 closed pull request #2944:
URL: https://github.com/apache/bookkeeper/pull/2944
--
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] kezhuw commented on a change in pull request #2944: Fix wrong ledger id parse radix for index relocation file in IndexPersistenceMgr
Posted by GitBox <gi...@apache.org>.
kezhuw commented on a change in pull request #2944:
URL: https://github.com/apache/bookkeeper/pull/2944#discussion_r770397152
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/FileInfo.java
##########
@@ -549,6 +549,7 @@ public synchronized void moveToNewLocation(File newFile, long size) throws IOExc
}
fc = new RandomAccessFile(newFile, mode).getChannel();
lf = newFile;
+ deleted = false;
Review comment:
Seems that all access methods of `deleted` and `fc` are `synchronized`. So I think it is safe ?
Actually, I could keep `deleted` untouched by using `lf.delete()` directly to delete file instead of `FileInfo.delete` which is used as spying point to mimic crashing behavior in tests. But, this way I have to introduce an extra spying point to intercept. Should I go this way and make spying point more explicitly ?
--
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] nicoloboschi commented on a change in pull request #2944: Fix wrong ledger id parse radix for index relocation file in IndexPersistenceMgr
Posted by GitBox <gi...@apache.org>.
nicoloboschi commented on a change in pull request #2944:
URL: https://github.com/apache/bookkeeper/pull/2944#discussion_r770438189
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/FileInfo.java
##########
@@ -549,6 +549,7 @@ public synchronized void moveToNewLocation(File newFile, long size) throws IOExc
}
fc = new RandomAccessFile(newFile, mode).getChannel();
lf = newFile;
+ deleted = false;
Review comment:
yes, the correct behavior would be to call `lf.delete()` directly because the `FileInfo` is an abstraction and `moveToNewLocation` is not expected the be in `deleted` state at all during the rename.
--
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] dlg99 commented on a change in pull request #2944: Fix wrong ledger id parse radix for index relocation file in IndexPersistenceMgr
Posted by GitBox <gi...@apache.org>.
dlg99 commented on a change in pull request #2944:
URL: https://github.com/apache/bookkeeper/pull/2944#discussion_r818130417
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/FileInfo.java
##########
@@ -549,6 +549,7 @@ public synchronized void moveToNewLocation(File newFile, long size) throws IOExc
}
fc = new RandomAccessFile(newFile, mode).getChannel();
lf = newFile;
+ deleted = false;
Review comment:
@nicoloboschi are you ok with 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@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] kezhuw commented on a change in pull request #2944: Fix wrong ledger id parse radix for index relocation file in IndexPersistenceMgr
Posted by GitBox <gi...@apache.org>.
kezhuw commented on a change in pull request #2944:
URL: https://github.com/apache/bookkeeper/pull/2944#discussion_r823266442
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/FileInfo.java
##########
@@ -549,6 +549,7 @@ public synchronized void moveToNewLocation(File newFile, long size) throws IOExc
}
fc = new RandomAccessFile(newFile, mode).getChannel();
lf = newFile;
+ deleted = false;
Review comment:
> `moveToNewLocation` is not expected the be in deleted state at all during the rename
It is only correct in ideal situation. Could this operation happen cross filesystems or even devices ? I think this method make no assumption about this. From my side, `delete` in `moveToNewLocation` is more like a conservative operation than solely `Files.delete()`(which throw exception on deletion failure). After failure of `FileInfo.delete`, `FileInfo` is unusable as `FileInfo.fc` is closed and `FileInfo.lf` is not guaranteed to exist.
I have no preference from approaches:
* Current: `FileInfo.delete()` and throws exception on failure.
* Alternative: `FileInfo.lf.delete()` and mark `deleted` and throws exception on failure.
I think they are identical.
--
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] kezhuw commented on pull request #2944: Fix wrong ledger id parse radix for index relocation file in IndexPersistenceMgr
Posted by GitBox <gi...@apache.org>.
kezhuw commented on pull request #2944:
URL: https://github.com/apache/bookkeeper/pull/2944#issuecomment-1008498598
@dlg99 I have fixed CI failures. The CI workflow has passed in my local repository. Could you please trigger a CI workflow run again for this pr ? 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@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org