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

Fix statistics in RocksJava sample #2674

Closed
wants to merge 1 commit into from

Conversation

sagar0
Copy link
Contributor

@sagar0 sagar0 commented Aug 1, 2017

I observed while doing a make jtest that the java sample was broken, due to the changes in #2551 .

Test Plan:
Before the fix:

cd rocksdb/java
make sample
javac -Xlint:deprecation -Xlint:unchecked -cp target/classes -d samples/target/classes samples/src/main/java/RocksDBSample.java
samples/src/main/java/RocksDBSample.java:44: error: cannot find symbol
            .createStatistics()
            ^
  symbol:   method createStatistics()
  location: class Options
samples/src/main/java/RocksDBSample.java:54: error: cannot find symbol
      final Statistics stats = options.statisticsPtr();
                                      ^
  symbol:   method statisticsPtr()
  location: variable options of type Options
samples/src/main/java/RocksDBSample.java:207: warning: [deprecation] remove(byte[]) in RocksDB has been deprecated
        db.remove(testKey);
          ^
2 errors
1 warning
make[1]: *** [sample] Error 1

After the fix:
No errors on doing a make jtest or make sample

@@ -31,6 +31,7 @@ public static void main(final String[] args) {
final Filter bloomFilter = new BloomFilter(10);
final ReadOptions readOptions = new ReadOptions()
.setFillCache(false);
final Statistics stats = new Statistics();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks good. However Statistics is a RocksObject and so should ideally be manually closed, along with ReadOptions. Probably best to put them in a try-with-resources

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But both Statistics and ReadOptions are already in try-with-resources here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry you are of course correct. I did not see it initially in the GitHub diff view. Sorry.

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.

Just needs a try-with-resources

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.

Looks good.

@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 sagar0 deleted the fix-java-sample branch August 3, 2017 17:39
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.

None yet

3 participants