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

ShardedLmdb takes max size in bytes, not pages #8038

Merged

Conversation

@illicitonion
Copy link
Contributor

commented Jul 11, 2019

No description provided.

@illicitonion illicitonion requested a review from blorente Jul 11, 2019

@blorente
Copy link
Contributor

left a comment

Much clearer to specify this with bytes :)

dir,
fingerprint_prefix,
));
}
Ok(envs)
}

fn make_env(dir: &Path, max_size: usize) -> Result<Environment, String> {
fn make_env(dir: &Path, max_size_bytes: usize) -> Result<Environment, String> {

This comment has been minimized.

Copy link
@blorente

blorente Jul 11, 2019

Contributor

page_size seems like it would be relevant when doing perf tuning and benchmarking, could we add a log statement that logged the value as a debug when we create an lmdb? Something like:

Suggested change
fn make_env(dir: &Path, max_size_bytes: usize) -> Result<Environment, String> {
fn make_env(dir: &Path, max_size_bytes: usize) -> Result<Environment, String> {
debug!("Creating environment with page size {}", page_size::get());

(assuming calls to page_size are both idempotent and cheap)

This comment has been minimized.

Copy link
@illicitonion

illicitonion Jul 11, 2019

Author Contributor

I'll hold off on adding this until we want the data for something specific :)

@stuhood
Copy link
Member

left a comment

Thanks!

@illicitonion illicitonion merged commit 503f17a into pantsbuild:master Jul 11, 2019

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details

illicitonion added a commit to twitter/pants that referenced this pull request Jul 11, 2019

Re-grow lmdb store sizes
pantsbuild#8038 shrunk them a fair whack, and probably shouldn't have.

I ran into an out-of-size since.

stuhood added a commit that referenced this pull request Jul 11, 2019

stuhood added a commit that referenced this pull request Jul 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.