KAFKA-14432: RocksDBStore relies on finalizers to not leak memory by lucasbru · Pull Request #12935 · apache/kafka

lucasbru · 2022-12-01T16:29:07Z

RocksDBStore relied on finalizers to not leak memory (and leaked memory after the upgrade to RocksDB 7).

The problem was that every call to options.statistics creates a new wrapper object that needs to be finalized.

I simplified the logic a bit and moved the ownership of the statistics from ValueProvider to RocksDBStore.

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

RocksDBStore relied on finalizers to not leak memory (and leaked memory after the upgrade to RocksDB 7). The problem was that every call to `options.statistics` creates a new wrapper object that needs to be finalized. I simplified the logic a bit and moved the ownership of the statistics from `ValueProvider` to `RocksDBStore`.

lucasbru · 2022-12-02T13:49:50Z

@cadonna Could you have a look? I ran this for a few hours in the soak and the native memory is stable. So I think this was the main leak

cadonna

@lucasbru Thanks for the PR!

Nice finding!

Here my feedback!

cadonna · 2022-12-05T15:33:30Z

+        statistics = userSpecifiedOptions.statistics();
+        if (statistics == null) {
+            if (RecordingLevel.forName((String) configs.get(METRICS_RECORDING_LEVEL_CONFIG)) == RecordingLevel.DEBUG) {
+                statistics = new Statistics();
+                dbOptions.setStatistics(statistics);
+            }
+            userSpecifiedStatistics = false;
+        } else {
+            userSpecifiedStatistics = true;
+        }


Could you extract this to a method? IMO, openDB() is already quite long.

For me personally, spreading linear code across methods with only one call-site does not help readability. Anyway, going with the PMC member on this one, done ;)

While I get the joke, it is not a matter of PMC member or not. If you have good arguments, I am happy to be convinced.

When I read code, I do not want to be distracted by every detail right away. For the first overview when reading a method -- for me -- it is enough to know that the statistics are set up. So reading setupStatistics() would be enough for me. Then, if I need to understand how the statistics are setup I would look into setupStatistics().

Yes, I was just joking. Normally, I'd be happy to convince you with arguments, but I think in this case it's really a matter of personal preference or convention among the engineers in the project (and you know those better, honestly!).

In this particular case, I think a comment and newlines help to split the function into correlated "blocks".

// setup statistics ... ... // next block

Arguably, this is not as clearly scoped as a separate method, as you say. But it does give me extra information when reading: I do not at all have to worry about this code if I am trying to understand any of the other methods, because the code cannot be called from other methods. Also, I don't have to use my IDE to jump around to understand the flow of execution.

Everyone will make these kinds of trade-offs slightly differently, I am happy to err on the side of project convention here.

I am not a big fan of comments. They eventually start to lie because they are not maintained as well as executable parts of the code. I prefer to extract a code block into a method with a meaningful name instead of using a inline comment. One could argue, that the name of a method might also start to lie, but IMO that is less likely compared to an inline comment.

yeah watch out @lucasbru, don't mention code comments around @cadonna or he'll come after you 😜

No comment!

cadonna · 2022-12-05T15:37:00Z

+        if (statistics != null) {
+            statistics.close();
+        }



What happens if a user-specified statistics object is closed by user code?

The user still needs to close the statistics he provided. This will just close the statistics handle.

I added a test for this. It doesn't check everything, but that is hard to do.

The user still needs to close the statistics he provided

We should add this to the docs then -- users forget to close things all the time, especially if we don't explicitly call out common objects that will need this. Can you file a followup ticket for this? Specifically we should definitely mention it in the rocksdb section of the Memory Management docs, but we also give an example RocksDBConfigSetter in the configuration docs where we might want to include this

Btw -- why/when would a user want to specify the statistics themselves vs just letting Streams handle it?

And on a related note, is there any chance failing to close this (either by the user or by Streams) might be causing some of the recent reports of getting no results and just seeing 0.0 for a bunch of the rocksdb metrics? I just feel like I've started seeing these pop up again and can't tell if there's a bug in user code or somehow in ours

I think you documented it already in the javadoc for close #6697

Sorry, I meant the "real" docs on the actual kafka site, not the javadocs. Seems like it would be useful to show how/why to use your own Statistics like this. Anyways I just filed a ticket for this so someone else can pick it up https://issues.apache.org/jira/browse/KAFKA-14459

cadonna · 2022-12-05T15:38:29Z

-    @Test
-    public void shouldCloseStatisticsWhenValueProvidersAreRemoved() {
-        recorder.addValueProviders(SEGMENT_STORE_NAME_1, dbToAdd1, cacheToAdd1, statisticsToAdd1);
-        recorder.removeValueProviders(SEGMENT_STORE_NAME_1);
-        verify(statisticsToAdd1).close();
-    }
-
-    @Test
-    public void shouldNotCloseStatisticsWhenValueProvidersWithoutStatisticsAreRemoved() {
-        recorder.addValueProviders(SEGMENT_STORE_NAME_1, dbToAdd1, cacheToAdd1, null);
-        recorder.removeValueProviders(SEGMENT_STORE_NAME_1);
-        verify(statisticsToAdd1, never()).close();
-    }


Is it possible to write some tests for RocksDBStore to ensure the statistics object is closed and the same instance is passed to the metrics recorder?

cadonna · 2022-12-05T15:40:00Z

@lucasbru Actually, I didn't want to approve. Could you please answer my questions before I merge?

lucasbru · 2022-12-06T13:53:57Z

Failing tests are unrelated

cadonna

@lucasbru Thanks for the updates!

Here my comments!

cadonna · 2022-12-07T11:02:12Z

-    private Statistics statistics;
+
+    // visible for testing
+    protected Statistics statistics;


I tried to avoid this with mocking the Statistics constructor with no luck.
However, I managed with reflection.

@Test public void shouldCloseStatisticsWhenUserProvidesNoStatistics() throws Exception { rocksDBStore = getRocksDBStoreWithRocksDBMetricsRecorder(); context = getProcessorContext(RecordingLevel.DEBUG); rocksDBStore.openDB(context.appConfigs(), context.stateDir()); Statistics statistics = getStatistics(rocksDBStore); rocksDBStore.close(); assertFalse(statistics.isOwningHandle()); statistics = getStatistics(rocksDBStore); assertNull(statistics); } private Statistics getStatistics(final RocksDBStore rocksDBStore) throws Exception { final Field statisticsField = rocksDBStore.getClass().getDeclaredField("statistics"); statisticsField.setAccessible(true); final Statistics statistics = (Statistics) statisticsField.get(rocksDBStore); statisticsField.setAccessible(false); return statistics; }

Normally, I am not a big fan of using reflection, but in this case were we need to verify off-heap memory leaks I think it is fine.

In such a way, we can keep statistics as a private field and still verify that there is not memory leak.

WDYT?

Great idea, thanks! Done

…/RocksDBStoreTest.java Co-authored-by: Bruno Cadonna <cadonna@apache.org>

cadonna

Thanks for the updates! Once the buiilds are acceptable, I will merge this PR!

ableegoldman · 2022-12-08T02:23:15Z

I did look over this and it seems reasonable but I'll refer to @cadonna's evaluation here as the expert on RocksDB metrics/statistics. Since he approved and the build passed (modulo a few unrelated/flaky test failures) I'm going to merge this and cherrypick back to 3.4

If there are any other/older branches you think this should go to, I'll leave it to @cadonna to port the fix as I suspect there could be nontrivial merge conflicts

…2935) RocksDBStore relied on finalizers to not leak memory (and leaked memory after the upgrade to RocksDB 7). The problem was that every call to options.statistics creates a new wrapper object that needs to be finalized. I simplified the logic a bit and moved the ownership of the statistics from ValueProvider to RocksDBStore. Reviewers: Bruno Cadonna <cadonna@apache.org>, Anna Sophie Blee-Goldman <ableegoldman@apache.org>, Christo Lolov <lolovc@amazon.com>

ableegoldman · 2022-12-08T02:27:39Z

Merged to trunk and cherrypicked to 3.4

lucasbru · 2022-12-08T11:42:55Z

If there are any other/older branches you think this should go to, I'll leave it to @cadonna to port the fix as I suspect there could be nontrivial merge conflicts

Yes, hmm we probably have to port this back to all branches that got the RocksDB upgrade, right? @cadonna

cadonna · 2022-12-08T13:36:34Z

I agree! I will backport this commit.

…2935) RocksDBStore relied on finalizers to not leak memory (and leaked memory after the upgrade to RocksDB 7). The problem was that every call to options.statistics creates a new wrapper object that needs to be finalized. I simplified the logic a bit and moved the ownership of the statistics from ValueProvider to RocksDBStore. Reviewers: Bruno Cadonna <cadonna@apache.org>, Anna Sophie Blee-Goldman <ableegoldman@apache.org>, Christo Lolov <lolovc@amazon.com>

cadonna · 2022-12-08T19:57:58Z

Backported to 3.3, 3.2, 3.1, and 3.0.

…ache#12935) RocksDBStore relied on finalizers to not leak memory (and leaked memory after the upgrade to RocksDB 7). The problem was that every call to options.statistics creates a new wrapper object that needs to be finalized. I simplified the logic a bit and moved the ownership of the statistics from ValueProvider to RocksDBStore. Reviewers: Bruno Cadonna <cadonna@apache.org>, Anna Sophie Blee-Goldman <ableegoldman@apache.org>, Christo Lolov <lolovc@amazon.com>

lucasbru force-pushed the leak_fix branch from 72576ed to 3d09be1 Compare December 2, 2022 08:59

clolov approved these changes Dec 2, 2022

View reviewed changes

lucasbru marked this pull request as ready for review December 2, 2022 13:47

cadonna approved these changes Dec 5, 2022

View reviewed changes

lucasbru added 2 commits December 6, 2022 11:51

address Brunos comments

6961722

Merge branch 'trunk' into leak_fix

6361a8f

cadonna reviewed Dec 7, 2022

View reviewed changes

lucasbru and others added 2 commits December 7, 2022 13:07

Update streams/src/test/java/org/apache/kafka/streams/state/internals…

6472b8d

…/RocksDBStoreTest.java Co-authored-by: Bruno Cadonna <cadonna@apache.org>

Use reflection to access private field

fa1b7c0

cadonna approved these changes Dec 7, 2022

View reviewed changes

ableegoldman merged commit 36a2f7b into apache:trunk Dec 8, 2022

cadonna mentioned this pull request May 9, 2023

KAFKA-14324: Upgrade RocksDB to 7.1.2 #12809

Merged

Conversation

lucasbru commented Dec 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committer Checklist (excluded from commit message)

Uh oh!

lucasbru commented Dec 2, 2022

Uh oh!

cadonna left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cadonna commented Dec 5, 2022

Uh oh!

lucasbru commented Dec 6, 2022

Uh oh!

cadonna left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lucasbru Dec 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cadonna left a comment

Choose a reason for hiding this comment

Uh oh!

ableegoldman commented Dec 8, 2022

Uh oh!

ableegoldman commented Dec 8, 2022

Uh oh!

lucasbru commented Dec 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cadonna commented Dec 8, 2022

Uh oh!

cadonna commented Dec 8, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lucasbru commented Dec 1, 2022 •

edited

Loading

lucasbru Dec 7, 2022 •

edited

Loading

lucasbru commented Dec 8, 2022 •

edited

Loading