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

Synchronize statistic enumeration values between statistics.h and java API #2209

Closed
wants to merge 1 commit into from

Conversation

bentorfs
Copy link
Contributor

@bentorfs bentorfs commented Apr 25, 2017

(Copied from #2171 ).
Problem: The enumerations of tickers and histogram types for retrieving statistics are not in sync between the C++ source and the java API:

Java version:
https://github.com/facebook/rocksdb/blob/master/java/src/main/java/org/rocksdb/TickerType.java
https://github.com/facebook/rocksdb/blob/master/java/src/main/java/org/rocksdb/HistogramType.java
C++ version:
https://github.com/facebook/rocksdb/blob/master/include/rocksdb/statistics.h

The java Enumeration is missing a lot of declarations, and their values do not correspond to the (implicit) order of values in the C++ enum.

Impact: for example, retrieving the BLOOM_FILTER_USEFUL statistic through the java API, actually returns the data for BLOCK_CACHE_FILTER_MISS.

@adamretter
Copy link
Collaborator

@bentorfs This looks good to me in that it fixes the issue.

However, as a more robust solution we should not rely on implicit enumeration values. For other enums that are similar in C++ and Java we have used an explicit mapping in portal.h, for an example see: https://github.com/facebook/rocksdb/blob/master/java/rocksjni/portal.h#L243. I think it would be good to do the same thing here really.

@dflemstr
Copy link

@adamretter @bentorfs It would be really great to see this merged, since it improves the current situation, and then we can talk about a nicer way of keeping things in sync later.

I'm not convinced that the status code example solves the problem because it relies on there not being anybody who tampers with the Java enum. I guess that is less likely than somebody unknowingly tampering with the C++ enum though.

@facebook-github-bot
Copy link
Contributor

@bentorfs updated the pull request - view changes

@bentorfs
Copy link
Contributor Author

Rebased to master

@sagar0
Copy link
Contributor

sagar0 commented Jun 14, 2017

I recently had to fix something like this in #2429. I wasn't aware of this PR while working on #2429, and got to know about this only today when it got bumped to the top due to today's comments and updates.

I do acknowledge @adamretter 's concern that there could be a more robust solution, but until someone implements it, this looks good to me. #UpForGrabs.

@facebook-github-bot
Copy link
Contributor

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

sagar0 pushed a commit that referenced this pull request Jun 27, 2017
…a API

Summary: Closes #2209

Differential Revision: D5251951

Pulled By: sagar0

fbshipit-source-id: 03a73d025a7b4a322bb8d8d86f5d249fcd7dd00e
adamretter added a commit to adamretter/rocksdb that referenced this pull request Jul 8, 2017
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.

5 participants