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

JNI: Do not create 8M block cache for negative blockCacheSize values #5465

Closed
wants to merge 1 commit into from

Conversation

javeme
Copy link
Contributor

@javeme javeme commented Jun 17, 2019

As BlockBasedTableConfig setBlockCacheSize() said, If cacheSize is non-positive, then cache will not be used. but when we configure a negative number or 0, there is an unexpected result: the block cache becomes 8M.

  • Allow 0 as a valid size. When block cache size is 0, an 8MB block cache is created, as it is the default C++ API behavior. Also updated the comment.
  • Set no_block_cache true if negative value is passed to block cache size, and no block cache will be created.

Copy link
Collaborator

@adamretter adamretter left a comment

Choose a reason for hiding this comment

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

LGTM. Although is 0 actually a valid choice? If so we should preserve 0

@sagar0
Copy link
Contributor

sagar0 commented Jun 19, 2019

0 is a valid size to block cache and I feel in such a case we should fall back to the default behavior on the C++ API side, which is to create a 8MB block cache when block_cache is set to nullptr.
If you don't want the block cache to be used, you need to use setNoBlockCache(true).

// Disable block cache. If this is set to true,
// then no block cache should be used, and the block_cache should
// point to a nullptr object.
bool no_block_cache = false;
// If non-NULL use the specified cache for blocks.
// If NULL, rocksdb will automatically create and use an 8MB internal cache.
std::shared_ptr<Cache> block_cache = nullptr;

* allowed 0 as a valid size and updated the comment.
* set no_block_cache true if negative number is passed.
@javeme
Copy link
Contributor Author

javeme commented Jun 24, 2019

@sagar0 @adamretter Thanks for the review, I have updated the code:

  1. allowed 0 as a valid size and updated the comment.
  2. set no_block_cache true if negative number is passed.

@adamretter
Copy link
Collaborator

@javeme LGTM

@sagar0 sagar0 changed the title JNI: fix 8M blockCacheSize with non-positive value JNI: Do not create 8M block cache for negative blockCacheSize values Jun 24, 2019
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@sagar0 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@sagar0 merged this pull request in c92c58f.

merryChris pushed a commit to merryChris/rocksdb that referenced this pull request Nov 18, 2019
…acebook#5465)

Summary:
As [BlockBasedTableConfig setBlockCacheSize()](https://github.com/facebook/rocksdb/blob/1966a7c055f6e182d627275051f5c09441aa922d/java/src/main/java/org/rocksdb/BlockBasedTableConfig.java#L728) said, If cacheSize is non-positive, then cache will not be used. but when we configure a negative number or 0, there is an unexpected result: the block cache becomes 8M.

- Allow 0 as a valid size. When block cache size is 0, an 8MB block cache is created, as it is the default C++ API behavior. Also updated the comment.
- Set no_block_cache true if negative value is passed to block cache size, and no block cache will be created.
Pull Request resolved: facebook#5465

Differential Revision: D15968788

Pulled By: sagar0

fbshipit-source-id: ee02d6e95841c9e2c316a64bfdf192d46ff5638a
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.

4 participants