-
Notifications
You must be signed in to change notification settings - Fork 895
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
Tuning pool concurrency #3432
Tuning pool concurrency #3432
Conversation
Object writeCacheConf = this.getProperty(DbLedgerStorage.WRITE_CACHE_MAX_SIZE_MB); | ||
if (writeCacheConf instanceof Number) { | ||
writeCacheMb = ((Number) writeCacheConf).longValue(); | ||
} else if (writeCacheConf == null) { | ||
writeCacheMb = DbLedgerStorage.DEFAULT_WRITE_CACHE_MAX_SIZE_MB; | ||
} else if (StringUtils.isEmpty(this.getString(DbLedgerStorage.WRITE_CACHE_MAX_SIZE_MB))) { | ||
writeCacheMb = DbLedgerStorage.DEFAULT_WRITE_CACHE_MAX_SIZE_MB; | ||
} else { | ||
writeCacheMb = this.getLong(DbLedgerStorage.WRITE_CACHE_MAX_SIZE_MB); | ||
} |
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 think all this can be done better by moving
long writeCacheMaxSize = getLongVariableOrDefault(conf, WRITE_CACHE_MAX_SIZE_MB,
DEFAULT_WRITE_CACHE_MAX_SIZE_MB) * MB;
long readCacheMaxSize = getLongVariableOrDefault(conf, READ_AHEAD_CACHE_MAX_SIZE_MB,
DEFAULT_READ_CACHE_MAX_SIZE_MB) * MB;
boolean directIOEntryLogger = getBooleanVariableOrDefault(conf, DIRECT_IO_ENTRYLOGGER, false);
and related config magic out of DbLedgerStorage into the ServerConfig
and then all you'd need to do is something like this.getWriteCacheMaxSize()
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.
Good idea.
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.
Good suggestion, I green with @dlg99
-
move DbLedgerStorage.DEFAULT_WRITE_CACHE_MAX_SIZE_MB to ServerConfiguration.DEFAULT_WRITE_CACHE_MAX_SIZE_MB
-
use new code
long writeCacheMaxSize = getLongVariableOrDefault(conf, WRITE_CACHE_MAX_SIZE_MB, conf.getAllocatorPoolingConcurrency()) * MB;
long readCacheMaxSize = getLongVariableOrDefault(conf, READ_AHEAD_CACHE_MAX_SIZE_MB, conf.getAllocatorPoolingConcurrency()) * MB;
instead of
long writeCacheMaxSize = getLongVariableOrDefault(conf, WRITE_CACHE_MAX_SIZE_MB, DEFAULT_WRITE_CACHE_MAX_SIZE_MB) * MB;
long readCacheMaxSize = getLongVariableOrDefault(conf, READ_AHEAD_CACHE_MAX_SIZE_MB, DEFAULT_READ_CACHE_MAX_SIZE_MB) * MB;
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.
can you add a testcase to cover the new function?
# Conflicts: # bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ServerConfiguration.java
|
||
this.allocator = allocator; | ||
this.numberOfDirs = ledgerDirsManager.getAllLedgerDirs().size(); | ||
|
||
log.info("Started Db Ledger Storage"); | ||
log.info(" - Number of directories: {}", numberOfDirs); | ||
log.info(" - Write cache size: {} MB", writeCacheMaxSize / MB); | ||
log.info(" - Read Cache: {} MB", readCacheMaxSize / MB); | ||
log.info(" - Write cache size: {} MB", writeCacheMaxSize / 1024 / 1024); |
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.
Could we define MB
?
public int getAllocatorPoolingConcurrency() { | ||
long writeCacheSize = this.getWriteCacheMaxSize(); | ||
long readCacheSize = this.getReadCacheMaxSize(); | ||
long availableDirectMemory = PlatformDependent.maxDirectMemory() - writeCacheSize - readCacheSize; |
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.
Please use PlatformDependent.estimateMaxDirectMemory()
instead of PlatformDependent.maxDirectMemory()
. Please refer to: #2989
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.
In dbLedgerStorage,
- If we use
DefaultEntryLogger
, only the whole write-cache will be allocated at one time in the init state, and the read-cache will be allocated on-demand. - If we use
DirectEnryLogger
, both the write-cache and the read-cache are allocated on-demand.
So I think we'd better keep the default implementation.
fix old workflow,please see #3455 for detail |
Descriptions of the changes in this PR:
This is a patch #3001. Consider unpooled direct memory on DbLedgerStorage.