-
Notifications
You must be signed in to change notification settings - Fork 903
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
BP-47 (task7): DbLedgerStorage add direct entry logger support #3366
BP-47 (task7): DbLedgerStorage add direct entry logger support #3366
Conversation
rerun failure checks |
1 similar comment
rerun failure checks |
rerun failure checks |
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.
Overall LGTM.
Please use PlatformDependent.estimateMaxDirectMemory instead of PlatformDependent.maxDirectMemory, for context see discussion on linked PR
@@ -91,6 +106,16 @@ public class DbLedgerStorage implements LedgerStorage { | |||
static final String READ_AHEAD_CACHE_BATCH_SIZE = "dbStorage_readAheadCacheBatchSize"; | |||
private static final int DEFAULT_READ_AHEAD_CACHE_BATCH_SIZE = 100; | |||
|
|||
private static final long DEFAULT_DIRECT_IO_TOTAL_WRITEBUFFER_SIZE_MB = | |||
(long) (0.125 * PlatformDependent.maxDirectMemory()) |
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.
PlatformDependent.estimateMaxDirectMemory()
See #2989
9a34b10
to
ff139a5
Compare
rerun failure checks |
Ping @merlimat @eolivelli @ivankelly @Vanlightly Please help take a look, thanks a lot. |
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.
Overall it looks good.
I left a small comment, but important, for the test
@Before | ||
public void setup() throws Exception { | ||
super.setup(); | ||
conf.setProperty("dbStorage_directIOEntryLogger", true); |
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.
Is it possible to add some assertions in the tests about the fact that we really loaded this implementation?
The tests may pass because we aren't using the new implementation
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.
done
rerun failure checks |
3 similar comments
rerun failure checks |
rerun failure checks |
rerun failure checks |
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.
👍
Motivation
Task 7 of BP-47, DbLedgerStorage add direct entry logger support.
This is the last Pr of BP-47
Modification
Add parameter
dbStorage_directIOEntryLogger
to control whether use direct entrylogger for DbLedgerStorage or not. Default value is false.Others
The commit is made by @ivankelly . This is the sub-task of #2932 , which was pushed out by @mauricebarnum. However, this PR contains too many changes and the number of code lines reaches 8K+, and it is hard to review. According to your suggestion #2932 (comment), and after communicating with @mauricebarnum, we are planning to divide the PR into 6 PRs, Please refer to #2943 (comment).
However, @mauricebarnum doesn't have enough time to deal with those issues, and we desperately need this feature. After communicating with @mauricebarnum and @merlimat , I help to work on dividing the PRs and pushing them out. We are hoping to contain this feature in BookKeeper 4.16.0