Skip to content
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

Set default sizes of DbLedgerStorage read and write cache to be proportional to JVM direct memory #1813

Merged
merged 4 commits into from
Nov 30, 2018

Conversation

merlimat
Copy link
Contributor

@merlimat merlimat commented Nov 15, 2018

Motivation

To simplify Bookie configuration when using DbLedgerStorage, set the memory size defaults for WriteCache, ReadCache and RocksDB block cache to be pegged to the available direct memory configured in the JVM.

User can always configure specific values and override this behavior.

@merlimat merlimat added this to the 4.9.0 milestone Nov 15, 2018
@merlimat merlimat self-assigned this Nov 15, 2018
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be change this default config file and explain how the value is calculated ?

Overall the change is good, just a few nits

@@ -24,6 +24,10 @@

import com.google.common.primitives.UnsignedBytes;

//CHECKSTYLE.OFF: IllegalImport
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment can be removed as well

@eolivelli
Copy link
Contributor

Mmm
But if we wat to use it we should remove it from the blacklist.
Otherwise who reads checkstyle config thinks that we are not using it...

What do you think?
Do you know why it was blacklisted?

@merlimat
Copy link
Contributor Author

What do you think?
Do you know why it was blacklisted?

I have no idea why it was blacklisted. I guess io.netty.util.internal could be regarded as non-public API package. Maybe it's because there are also some shaded classes under that package.

In my opinion, the override here is fine. We should only using things from io.netty.util.internal if there's a good reason for it and no other way around.

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good

#shipit

@sijie
Copy link
Member

sijie commented Nov 16, 2018

the reason why it is in blacklist is internal classes are not encouraged to use in general, if there are exceptions, those imports should be highlighted with these checkstyle annotations. so we know these packages are used intentionally.

Copy link
Member

@sijie sijie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@merlimat : as we talked offline, I would like to have some simple validation in the server configuration, if the total memory size configured to be used for db ledger storage exceed the jvm memory, we should fail startup with an explicit logging.

@merlimat
Copy link
Contributor Author

@sijie Updated

@merlimat
Copy link
Contributor Author

run integration tests

@sijie sijie changed the title Set default sizes of DbLedgerStorage read and write cache to be propo… Set default sizes of DbLedgerStorage read and write cache to be proportional to JVM direct memory Nov 30, 2018
@sijie sijie merged commit 1285d20 into apache:master Nov 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants