Fix bk write failure part 2 #5322
Merged
+189
−63
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…bk_write_failure_part_2
merlimat
reviewed
Oct 5, 2019
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java
Show resolved
Hide resolved
jerrypeng
commented
Oct 6, 2019
managed-ledger/src/test/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerErrorsTest.java
Show resolved
Hide resolved
jerrypeng
force-pushed
the
fix_bk_write_failure_part_2
branch
from
474607e to
8fb9c7d
Compare
aahmed-se
approved these changes
Oct 6, 2019
|
run java8 tests |
merlimat
approved these changes
Oct 7, 2019
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java
Outdated
Show resolved
Hide resolved
managed-ledger/src/test/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerErrorsTest.java
Show resolved
Hide resolved
rdhabalia
reviewed
Oct 7, 2019
| if (exception instanceof ManagedLedgerFencedException) { | ||
| // If the managed ledger has been fenced, we cannot continue using it. We need to close and reopen | ||
| close(); | ||
| } else { | ||
|
|
||
| // fence topic when failed to write a message to BK | ||
| isFenced = true; |
There was a problem hiding this comment.
why are we avoiding fencing ledger when topic actually sees ManagedLedgerFencedException
There was a problem hiding this comment.
@rdhabalia I add this set here in a previous PR part of a line of PRs to fix problems in message deduplication (#5218). Setting "isFenced = true;" there when the exception is ManagedLedgerFencedException is inappropriate because of this check in close():
There is already logic in the close() method to fence the topic
|
rerun java8 tests |
|
rerun integration tests |
wolfstudy
pushed a commit
that referenced
this pull request
Nov 20, 2019
* Bug in Message Deduplication that may cause incorrect behavior * add tests * fix error message * fix client backoff * fix tests * cleaning up * Fix handling of BK write failures for message dedup * tests and clean up * refactoring code * fixing bugs * addressing comments * add missing license header * Improve error handling of BK write failures * fixing tests * fixing bugs * cleaning up * addressing comments * fixing tests (cherry picked from commit 60abcaa)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Master Issue: #5218
Motivation
Last PR to fix #5218 and a continuation of PR #5271. When a BK write failures happens, instead of waiting for 10 seconds before creating a new ledger, we should use proper signals to resume writing (create a new ledger) after the error recovery code has completed running to prevent any race conditions.