Add native logger support to RocksJava

[neilramaswamy]

Overview

In this PR, we introduce support for setting the RocksDB native logger through Java. As mentioned in the discussion on the Google Group discussion, this work is primarily motivated by the JDK 17 performance regression in JNI thread attach/detach calls: the only existing RocksJava logging configuration call, setLogger, invokes the provided logger over the JNI.

Changes

Specifically, these changes add support for the devnull and stderr native loggers. For the stderr logger, we add the ability to prefix every log with a logPrefix, so that it becomes possible know which database a particular log is coming from (if multiple databases are in use). The API looks like the following:

Options opts = new Options();

NativeLogger stderrNativeLogger = NativeLogger.newStderrLogger(
  InfoLogLevel.DEBUG_LEVEL, "[my prefix here]");
options.setLogger(stderrNativeLogger);

try (final RocksDB db = RocksDB.open(options, ...))  {...}

// Cleanup
stderrNativeLogger.close()
opts.close();

Note that the API to set the logger is the same, via Options::setLogger (or DBOptions::setLogger). However, it will set the RocksDB logger to be native when the provided logger is an instance of NativeLogger.

Testing

Two tests have been added in NativeLoggerTest.java. The first test creates both the devnull and stderr loggers, and sets them on the associated Options. However, to avoid polluting the testing output with logs from stderr, only the devnull logger is actually used in the test. The second test does the same logic, but for DBOptions.

It is possible to manually verify the stderr logger by modifying the tests slightly, and observing that the console indeed gets cluttered with logs from stderr.

[neilramaswamy]

Hi @adamretter, I'd love if you could take a look at these updates to RocksJava. Happy to make any changes as needed.

[adamretter]

@neilramaswamy I think we should stick with just setLogger(...) in the DBOptions (and friends) and not add a setNativeLogger(...). We have two options I think:

1. Rename Logger to AbstractLogger, and add a Logger Interface (which AbstractLogger should implement), leave setLogger(Logger).
2. Add an interface called LoggerInterface which Logger implements, and also modify setLogger(Logger) to setLogger(LoggerInterface).

Option 1 may break some existing applications but gives a cleaner API, whilst Option 2 is less disruptive but leaves some cruft in the existing API design. I would be happy with either approach.

With either approach, the Native Loggers you are interested in can each be represented by a Java class which implements Logger (or an AbstractClass implementation thereof), and setLogger() can handle both native and non-native loggers by checking for their sub-class.

[neilramaswamy]

@adamretter, thank you for your quick comments. I've implemented the second option, since I didn't want to break any existing code.

But yes, there certainly some cruft: the LoggerInterface is very thin, and it's less a logger interface than a hack to let us pass either a Logger or NativeLogger to setLogger. But if you think it's cleaner than having setNativeLogger, I'm happy with this.

To make sure the cruft doesn't get in the way of usability, I can add documentation to the RocksJava Wiki page on how to create a JVM logger and set it, as well as configuring a native logger and setting that.

[adamretter]

@neilramaswamy I have pushed a couple of commits to refactor this more towards the API that I had in mind. I think perhaps that my previous explanation wasn't very detailed. Let me know if you are happy with it?

[neilramaswamy]

This API works with me, but there will then end up being some redundancy in the LoggerInterface methods among the native loggers. Since there are very few more native loggers, this is probably fine.

[adamretter]

LGTM. Thanks :-)

[adamretter]

but there will then end up being some redundancy in the LoggerInterface methods among the native loggers

We can add an abstract sub-class in-future if needed I think @neilramaswamy

[adamretter]

@siying @ajkr If you are happy with this, then I am happy with this now.

[facebook-github-bot]

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

[facebook-github-bot]

@neilramaswamy has updated the pull request. You must reimport the pull request before landing.

[facebook-github-bot]

@neilramaswamy has updated the pull request. You must reimport the pull request before landing.

[neilramaswamy]

Thanks for the review @ajkr! I addressed the first issue, and I think I fixed the test failure (they were only Windows build errors; I unfortunately do not have a Windows machine, so I can't test locally). I will wait for CI and iterate from there.

[ajkr]

LGTM, thanks!

[facebook-github-bot]

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

[neilramaswamy]

Hi @ajkr, I believe you need to approve the workflow for all the checks to run (that's what GitHub tells me). I can take a look at any Windows build errors (if any) after that.

Also, I can't see the "Facebook Internal - Linter" check, which is failing. If you are able to share the warnings with me, I can fix them.

[ajkr]

Also, I can't see the "Facebook Internal - Linter" check, which is failing. If you are able to share the warnings with me, I can fix them.

The linter wants StderrLogger's constructor to take the prefix parameter by const-reference (I know this was discussed already and don't really care either way). It also asks for the semicolon after that same constructor's (empty) body to be dropped

[facebook-github-bot]

@neilramaswamy has updated the pull request. You must reimport the pull request before landing.

[neilramaswamy]

Alas, all tests must run again, but I removed the semi. I think passing the string directly is more clear, so I left it. The Windows errors were fixed, so this should be the last time you have to approve the workflow :)

Thanks for the engagement on this. I really appreciate it.

[facebook-github-bot]

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

[facebook-github-bot]

@neilramaswamy has updated the pull request. You must reimport the pull request before landing.

[facebook-github-bot]

@neilramaswamy has updated the pull request. You must reimport the pull request before landing.

[facebook-github-bot]

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

[facebook-github-bot]

@neilramaswamy has updated the pull request. You must reimport the pull request before landing.

[facebook-github-bot]

@neilramaswamy has updated the pull request. You must reimport the pull request before landing.

[facebook-github-bot]

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

[facebook-github-bot]

@ajkr merged this pull request in 4835c11.