[DLOG] Avoid double read in readahead #1973
Merged
Conversation
This file contains 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
There was a bug in the ReadAheadReader whereby, if it was in a paused state and the last segment it had read had been closed, then when a writer started writing new segments, the ReadAheadReader would issue a double read. This would end up triggering an error when it got to the end of the segment as the issued one of the issued reads would then be cancelled when the segment was closed. This fix adds an explicit state machine to the reader and moves all state transitions to run on the ordered executor, so only one read can ever be issued at a time.
|
@ivankelly did you see some error which is getting fixed by this patch ? I can't see any issue attached. Is this problem affecting production with 4.9 or is only a case that you found analysing the code ? cc @diegosalvi FYI |
|
is it #1970 ? |
|
@eolivelli the issue was observed by a customer running 4.7.3, but the code hasn't changed much since. The attached unit test will trigger it if you want to see the problem in action. |
eolivelli
requested changes
Mar 4, 2019
| @@ -67,6 +67,15 @@ | |||
|
|
|||
| private static final Logger logger = LoggerFactory.getLogger(ReadAheadEntryReader.class); | |||
|
|
|||
| enum State { | |||
| case READING: | ||
| case ERROR: | ||
| case CLOSED: | ||
| // do nothing |
There was a problem hiding this comment.
shall a log at trace level be useful ?
There was a problem hiding this comment.
nah, we already know if we're going to hit this branch from the debug log above.
eolivelli
approved these changes
Mar 4, 2019
sijie
approved these changes
Mar 4, 2019
|
please also cherry-pick the change to 4.9.1 and 4.8.3 |
|
run bookkeeper-server bookie tests |
ivankelly
added a commit
that referenced
this pull request
Mar 5, 2019
There was a bug in the ReadAheadReader whereby, if it was in a paused state and the last segment it had read had been closed, then when a writer started writing new segments, the ReadAheadReader would issue a double read. This would end up triggering an error when it got to the end of the segment as the issued one of the issued reads would then be cancelled when the segment was closed. This fix adds an explicit state machine to the reader and moves all state transitions to run on the ordered executor, so only one read can ever be issued at a time. Reviewers: Enrico Olivelli <eolivelli@gmail.com>, Sijie Guo <sijie@apache.org> This closes #1973 from ivankelly/dlog-reader-crash-master (cherry picked from commit db5d7e7) Signed-off-by: Ivan Kelly <ivank@apache.org>
ivankelly
added a commit
that referenced
this pull request
Mar 5, 2019
There was a bug in the ReadAheadReader whereby, if it was in a paused state and the last segment it had read had been closed, then when a writer started writing new segments, the ReadAheadReader would issue a double read. This would end up triggering an error when it got to the end of the segment as the issued one of the issued reads would then be cancelled when the segment was closed. This fix adds an explicit state machine to the reader and moves all state transitions to run on the ordered executor, so only one read can ever be issued at a time. Reviewers: Enrico Olivelli <eolivelli@gmail.com>, Sijie Guo <sijie@apache.org> This closes #1973 from ivankelly/dlog-reader-crash-master (cherry picked from commit db5d7e7) Signed-off-by: Ivan Kelly <ivank@apache.org>
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.
There was a bug in the ReadAheadReader whereby, if it was in a paused
state and the last segment it had read had been closed, then when a
writer started writing new segments, the ReadAheadReader would issue a
double read. This would end up triggering an error when it got to the
end of the segment as the issued one of the issued reads would then be
cancelled when the segment was closed.
This fix adds an explicit state machine to the reader and moves all
state transitions to run on the ordered executor, so only one read can
ever be issued at a time.