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

Configure BK memory sizes automatically from JVM parameters #3572

Closed
wants to merge 2 commits into from

Conversation

merlimat
Copy link
Contributor

Motivation

In BK 4.9, the DbLedgerStorage is already auto-tuning the mem sizes from the available memory in the JVM. Removing the fixed size default.

@merlimat merlimat added the type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages label Feb 11, 2019
@merlimat merlimat added this to the 2.3.0 milestone Feb 11, 2019
@merlimat merlimat self-assigned this Feb 11, 2019
@merlimat
Copy link
Contributor Author

run integration tests
run java8 tests

@jiazhai
Copy link
Member

jiazhai commented Feb 12, 2019

run integration tests

1 similar comment
@merlimat
Copy link
Contributor Author

run integration tests

@sijie
Copy link
Member

sijie commented Feb 13, 2019

run integration tests

@ivankelly
Copy link
Contributor

rerun integration tests

@ivankelly
Copy link
Contributor

We should enable -XX:+UseCGroupMemoryLimitForHeap for the docker images to ensure that if this is running on a large machine in k8s, that it doesn't try to pull a huge amount of memory.

@ivankelly
Copy link
Contributor

In fact, I'm going to hazard a guess that this is why integration tests are failing.

@ivankelly
Copy link
Contributor

Not quite, but the failure is related to this change.

@merlimat
Copy link
Contributor Author

The problem is that DbLedgerStorage is failing when the config key is present but empty:

14:50:13.375 [main] ERROR org.apache.bookkeeper.server.Main - Failed to build bookie server
org.apache.commons.configuration.ConversionException: 'dbStorage_writeCacheMaxSizeMb' doesn't map to a Long object
	at org.apache.commons.configuration.AbstractConfiguration.getLong(AbstractConfiguration.java:884) ~[commons-configuration-commons-configuration-1.6.jar:1.6]
	at org.apache.commons.configuration.AbstractConfiguration.getLong(AbstractConfiguration.java:865) ~[commons-configuration-commons-configuration-1.6.jar:1.6]
	at org.apache.bookkeeper.bookie.storage.ldb.DbLedgerStorage.initialize(DbLedgerStorage.java:97) ~[org.apache.bookkeeper-bookkeeper-server-4.9.0.jar:4.9.0]
	at org.apache.bookkeeper.bookie.Bookie.<init>(Bookie.java:790) ~[org.apache.bookkeeper-bookkeeper-server-4.9.0.jar:4.9.0]
	at org.apache.bookkeeper.proto.BookieServer.newBookie(BookieServer.java:137) ~[org.apache.bookkeeper-bookkeeper-server-4.9.0.jar:4.9.0]
	at org.apache.bookkeeper.proto.BookieServer.<init>(BookieServer.java:106) ~[org.apache.bookkeeper-bookkeeper-server-4.9.0.jar:4.9.0]
	at org.apache.bookkeeper.server.service.BookieService.<init>(BookieService.java:43) ~[org.apache.bookkeeper-bookkeeper-server-4.9.0.jar:4.9.0]
	at org.apache.bookkeeper.server.Main.buildBookieServer(Main.java:301) ~[org.apache.bookkeeper-bookkeeper-server-4.9.0.jar:4.9.0]
	at org.apache.bookkeeper.server.Main.doMain(Main.java:221) [org.apache.bookkeeper-bookkeeper-server-4.9.0.jar:4.9.0]
	at org.apache.bookkeeper.server.Main.main(Main.java:203) [org.apache.bookkeeper-bookkeeper-server-4.9.0.jar:4.9.0]
	at org.apache.bookkeeper.proto.BookieServer.main(BookieServer.java:314) [org.apache.bookkeeper-bookkeeper-server-4.9.0.jar:4.9.0]
Caused by: org.apache.commons.configuration.ConversionException: Could not convert  to java.lang.Long
	at org.apache.commons.configuration.PropertyConverter.toNumber(PropertyConverter.java:413) ~[commons-configuration-commons-configuration-1.6.jar:1.6]
	at org.apache.commons.configuration.PropertyConverter.toLong(PropertyConverter.java:280) ~[commons-configuration-commons-configuration-1.6.jar:1.6]
	at org.apache.commons.configuration.AbstractConfiguration.getLong(AbstractConfiguration.java:880) ~[commons-configuration-commons-configuration-1.6.jar:1.6]
	... 10 more

We want the key to be present (and empty) in the config file so that we can easily override in Docker environment.

Moving this to 2.3.1 since it will require a small change in BK side

@merlimat merlimat modified the milestones: 2.3.0, 2.3.1 Feb 14, 2019
@ivankelly
Copy link
Contributor

👍 we should take the Cgroup stuff into account in our images also though

@merlimat
Copy link
Contributor Author

Sure, but that can be a bit tricky to enable in a general way because:

  • CGroup mem limit might not have been set on the container, so we end up using all mem
  • UseCGroupMemoryLimitForHeap is basically setting the Xmx automatically based on the mem quota, but we also need to leave room for direct memory :/

@merlimat merlimat modified the milestones: 2.3.1, 2.4.0 Feb 22, 2019
@merlimat
Copy link
Contributor Author

This was already done in #4297

@merlimat merlimat closed this May 30, 2019
@merlimat merlimat deleted the bk-conf branch May 30, 2019 21:02
@merlimat merlimat removed this from the 2.4.0 milestone May 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants