support getUsage and getPinnedUsage in JavaAPI for Cache #7925
Conversation
|
these 2 methods are mentioned in https://github.com/facebook/rocksdb/wiki/Memory-usage-in-RocksDB |
| @@ -10,4 +10,16 @@ | |||
| protected Cache(final long nativeHandle) { | |||
| super(nativeHandle); | |||
| } | |||
| public long getUsage() { | |||
There was a problem hiding this comment.
Please add Javadoc to each public method.
There was a problem hiding this comment.
The sentence in the Javadoc description paragraph should start with a capital letter please.
| try(final Cache lruCache = new LRUCache(capacity, | ||
| numShardBits, strictCapacityLimit, highPriPoolRatio)) { | ||
| //no op | ||
| lruCache.getUsage(); |
There was a problem hiding this comment.
These need JUnit assertions on them please.
| lruCache.getUsage(); | ||
| lruCache.getPinnedUsage(); | ||
| } catch (Exception e) { | ||
| assert (false); |
There was a problem hiding this comment.
You can simply have the exception thrown by the method, this will then fail the test
|
Hi @adamretter, are you satisfied with the updates after your last review? |
There was a problem hiding this comment.
@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@adamretter are your comments all addressed now? |
| try(final Cache lruCache = new LRUCache(capacity, | ||
| numShardBits, strictCapacityLimit, highPriPoolRatio)) { | ||
| //no op | ||
| assert (lruCache.getUsage() >= 0); |
There was a problem hiding this comment.
There need to be test based assertions rather than system assertions. See the other tests for examples
| @@ -10,4 +10,16 @@ | |||
| protected Cache(final long nativeHandle) { | |||
| super(nativeHandle); | |||
| } | |||
| public long getUsage() { | |||
There was a problem hiding this comment.
The sentence in the Javadoc description paragraph should start with a capital letter please.
|
@ajkr sorry I didn't see your request of me 1 week ago. Looks much better, just really needs to use the correct assertions in the test now |
|
@pheecian has updated the pull request. You must reimport the pull request before landing. |
|
@adamretter made code refined, would you please review it again? |
There was a problem hiding this comment.
Thanks. Looks good to me now.
@mrambacher @ajkr Ready for merge I think
There was a problem hiding this comment.
@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
support getUsage and getPinnedUsage in JavaAPI for Cache
also fix a typo in LRUCacheTest.java that the highPriPoolRatio is not valid(set 5, I guess it means 0.05)