-
Notifications
You must be signed in to change notification settings - Fork 561
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(atomix): consume read buffer correctly #4372
Conversation
Fix issue where the read buffer was consumed only half and the rest was thrown away. Refactor FileChannelJournalSegmentReader#readNext to improve readability and hopefully maintainability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't quite understand what was the problem in the previous version. But now the code is more readable , so 👍 🙂
Can we add more tests for this in FileChannelSegmentReaderTest
? I could find the problem with reading checksum length by tweaking the existing test to read all events, write entries of type Long and set max entry size to 2*Long. I think that we have to test it with different configuration - entry size, max entry size etc. to find potential bugs here.
atomix/storage/src/main/java/io/atomix/storage/journal/FileChannelJournalSegmentReader.java
Outdated
Show resolved
Hide resolved
Adjust check such that it takes also checksum into account. Adjust tests such that they fail if this change is missing
Thanks @deepthidevaki for the review. I adjusted my PR and take the checksum length into account. I also changed to current tests such that they are failing if this check is missing. Furthermore I added one more test. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good. Just one minor comment.
atomix/storage/src/test/java/io/atomix/storage/journal/AbstractJournalTest.java
Show resolved
Hide resolved
bors r+ |
4372: fix(atomix): consume read buffer correctly r=Zelldon a=Zelldon ## Description Fix issue where the read buffer was consumed only half and the rest was thrown away. Refactor `FileChannelJournalSegmentReader#readNext` to improve readability and hopefully maintainability. <!-- Please explain the changes you made here. --> I run also a benchmark with these changes (#4358 was also part of the branch I run a benchmark with) ![metrics](https://user-images.githubusercontent.com/2758593/80199113-45202a00-8621-11ea-9595-e8c65130ec97.png) ## Related issues <!-- Which issues are closed by this PR or are related --> closes #4248 # Co-authored-by: Christopher Zell <zelldon91@googlemail.com>
Build failed |
Bors r+ |
Build succeeded |
Description
Fix issue where the read buffer was consumed only half and the rest was
thrown away.
Refactor
FileChannelJournalSegmentReader#readNext
to improve readability and hopefully maintainability.I run also a benchmark with these changes (#4358 was also part of the branch I run a benchmark with)
Related issues
closes #4248
Pull Request Checklist
mvn clean install -DskipTests
locally before committing