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/08/01 08:52:04 UTC
[GitHub] [bookkeeper] leizhiyuan opened a new pull request, #3437: feat: support skip invalid record in recovery
leizhiyuan opened a new pull request, #3437:
URL: https://github.com/apache/bookkeeper/pull/3437
Descriptions of the changes in this PR:
### Motivation
fix #2220
### Changes
(Describe: what changes you have made)
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] hangc0276 commented on pull request #3437: Issue #2220: support skip invalid record in recovery
Posted by "hangc0276 (via GitHub)" <gi...@apache.org>.
hangc0276 commented on PR #3437:
URL: https://github.com/apache/bookkeeper/pull/3437#issuecomment-1512409824
@leizhiyuan Would you please send a discuss to the @dev mail list to have a discuss? 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: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] leizhiyuan commented on a diff in pull request #3437: Issue #2220: support skip invalid record in recovery
Posted by GitBox <gi...@apache.org>.
leizhiyuan commented on code in PR #3437:
URL: https://github.com/apache/bookkeeper/pull/3437#discussion_r937414404
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieImpl.java:
##########
@@ -618,7 +618,7 @@ private void replay(Journal journal, JournalScanner scanner) throws IOException
logPosition = markedLog.getLogFileOffset();
}
LOG.info("Replaying journal {} from position {}", id, logPosition);
- long scanOffset = journal.scanJournal(id, logPosition, scanner);
+ long scanOffset = journal.scanJournal(id, logPosition, scanner, this.conf.isSkipReplayJournalInvalidRecord());
Review Comment:
what we meet is
a bk shutdown because of VM crash,then VM recovered , but we can not restart bk ， because of the exception。we can not skip this， we only can do format data for this VM.. and re install bk
so if we want to recovery the bk in the scene， we can open the switch only on the machine，it will startup, and next time， we will close this switch
--
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 #3437: Issue #2220: support skip invalid record in recovery
Posted by GitBox <gi...@apache.org>.
StevenLuMT commented on code in PR #3437:
URL: https://github.com/apache/bookkeeper/pull/3437#discussion_r936169723
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieImpl.java:
##########
@@ -618,7 +618,7 @@ private void replay(Journal journal, JournalScanner scanner) throws IOException
logPosition = markedLog.getLogFileOffset();
}
LOG.info("Replaying journal {} from position {}", id, logPosition);
- long scanOffset = journal.scanJournal(id, logPosition, scanner);
+ long scanOffset = journal.scanJournal(id, logPosition, scanner, this.conf.isSkipReplayJournalInvalidRecord());
Review Comment:
I have a question:
How do we decide to open the switch
What I understand is that open this switch ,maube cause to loss data,
does this need to discuss it on the dev@ mailing list @eolivelli , have a look,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
[GitHub] [bookkeeper] leizhiyuan commented on pull request #3437: feat: support skip invalid record in recovery
Posted by GitBox <gi...@apache.org>.
leizhiyuan commented on PR #3437:
URL: https://github.com/apache/bookkeeper/pull/3437#issuecomment-1201838010
> Does the old testcase cover this function, do you need to add a new testcase?
done
--
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 #3437: Issue #2220: support skip invalid record in recovery
Posted by GitBox <gi...@apache.org>.
StevenLuMT commented on PR #3437:
URL: https://github.com/apache/bookkeeper/pull/3437#issuecomment-1225336671
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 #3437: Issue #2220: support skip invalid record in recovery
Posted by "hangc0276 (via GitHub)" <gi...@apache.org>.
hangc0276 commented on code in PR #3437:
URL: https://github.com/apache/bookkeeper/pull/3437#discussion_r1169469405
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Journal.java:
##########
@@ -884,22 +885,31 @@ public long scanJournal(long journalId, long journalPos, JournalScanner scanner)
}
boolean isPaddingRecord = false;
if (len < 0) {
- if (len == PADDING_MASK && journalVersion >= JournalChannel.V5) {
- // skip padding bytes
- lenBuff.clear();
- fullRead(recLog, lenBuff);
- if (lenBuff.remaining() != 0) {
- break;
+ try {
+ if (len == PADDING_MASK && journalVersion >= JournalChannel.V5) {
+ // skip padding bytes
+ lenBuff.clear();
+ fullRead(recLog, lenBuff);
+ if (lenBuff.remaining() != 0) {
+ break;
+ }
+ lenBuff.flip();
+ len = lenBuff.getInt();
+ if (len == 0) {
+ continue;
+ }
+ isPaddingRecord = true;
+ } else {
+ LOG.error("Invalid record found with negative length: {}", len);
+ throw new IOException("Invalid record found with negative length " + len);
}
- lenBuff.flip();
- len = lenBuff.getInt();
- if (len == 0) {
- continue;
+ } catch (IOException e) {
+ if (skipInvalidRecord) {
Review Comment:
If we just skip one record and continue to read the next one, I'm afraid it will cause ledger data errors. Because when one journal file is broken, the rest of the data will be organized in the wrong way and we can't parse 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] leizhiyuan commented on a diff in pull request #3437: Issue #2220: support skip invalid record in recovery
Posted by GitBox <gi...@apache.org>.
leizhiyuan commented on code in PR #3437:
URL: https://github.com/apache/bookkeeper/pull/3437#discussion_r997868244
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Journal.java:
##########
@@ -897,7 +898,12 @@ public long scanJournal(long journalId, long journalPos, JournalScanner scanner)
continue;
}
isPaddingRecord = true;
- } else {
+ } else if (skipInvalidRecord){
+ LOG.warn("Invalid record found with negative length: {},because of " +
Review Comment:
ok
--
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 #3437: Issue #2220: support skip invalid record in recovery
Posted by GitBox <gi...@apache.org>.
hangc0276 commented on code in PR #3437:
URL: https://github.com/apache/bookkeeper/pull/3437#discussion_r997122159
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Journal.java:
##########
@@ -897,7 +898,12 @@ public long scanJournal(long journalId, long journalPos, JournalScanner scanner)
continue;
}
isPaddingRecord = true;
- } else {
+ } else if (skipInvalidRecord){
+ LOG.warn("Invalid record found with negative length: {},because of " +
Review Comment:
Maybe we can catch the exception before `finally`.
--
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 closed pull request #3437: Issue #2220: support skip invalid record in recovery
Posted by "hangc0276 (via GitHub)" <gi...@apache.org>.
hangc0276 closed pull request #3437: Issue #2220: support skip invalid record in recovery
URL: https://github.com/apache/bookkeeper/pull/3437
--
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] frankjkelly commented on pull request #3437: Issue #2220: support skip invalid record in recovery
Posted by "frankjkelly (via GitHub)" <gi...@apache.org>.
frankjkelly commented on PR #3437:
URL: https://github.com/apache/bookkeeper/pull/3437#issuecomment-1531407837
Bump
--
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 #3437: Issue #2220: support skip invalid record in recovery
Posted by "hangc0276 (via GitHub)" <gi...@apache.org>.
hangc0276 commented on PR #3437:
URL: https://github.com/apache/bookkeeper/pull/3437#issuecomment-1545468066
I will take over this PR /cc @leizhiyuan
--
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 #3437: Issue #2220: support skip invalid record in recovery
Posted by "hangc0276 (via GitHub)" <gi...@apache.org>.
hangc0276 commented on PR #3437:
URL: https://github.com/apache/bookkeeper/pull/3437#issuecomment-1547672781
@frankjkelly I have created a new PR to track it https://github.com/apache/bookkeeper/pull/3956
--
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 #3437: Issue #2220: support skip invalid record in recovery
Posted by "hangc0276 (via GitHub)" <gi...@apache.org>.
hangc0276 commented on PR #3437:
URL: https://github.com/apache/bookkeeper/pull/3437#issuecomment-1537121679
@leizhiyuan Do you have any updates?
--
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 #3437: Issue #2220: support skip invalid record in recovery
Posted by "hangc0276 (via GitHub)" <gi...@apache.org>.
hangc0276 commented on PR #3437:
URL: https://github.com/apache/bookkeeper/pull/3437#issuecomment-1605897464
The issue has been fixed by https://github.com/apache/bookkeeper/pull/3956, close this PR.
--
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