KAFKA-8324: Add close() method to RocksDBConfigSetter by ableegoldman · Pull Request #6697 · apache/kafka

ableegoldman · 2019-05-08T00:36:41Z

Following KIP-453, this PR adds a default close() method to the RocksDBConfigSetter interface and calls it when closing a store.

ableegoldman · 2019-05-08T00:40:07Z

@mjsax @abbccdda @guozhangwang @bbejeck @vvcephei @cadonna

cadonna

Thanks @ableegoldman
LGTM, mainly JavaDocs comments.

cadonna · 2019-05-08T11:25:39Z

+     *
+     * @param storeName     the name of the store being configured
+     * @param options       the Rocks DB options
+\    */


There is a \ too much, isn't it?

cadonna · 2019-05-08T11:28:10Z

+     * here to avoid leaking off-heap memory.
+     * Objects to be closed can be saved by the user or retrieved back from options using its getter methods.
+     *
+     * Example objects needing to be closed include Filter and Cache


I would use the full qualified name of Filter and Cache. If anybody wants to look them up it is easier. Additionally, I would enclose it into {@code ...}.

same... @link > @close for this case.

cadonna · 2019-05-08T11:29:00Z

+     * @param storeName     the name of the store being configured
+     * @param options       the Rocks DB options
+\    */
+    default void close(final String storeName, final Options options) {


Are there plans to remove the default implementation in the next major release?

That's a tricky question... The problem is that there's no way to signal ahead of time that you should override the method to avoid breakage when we remove default (unlike when you deprecate a method, which indicates you should make a change ahead of the breakage).

So, whenever we do make the change, it will be a breaking one.

If we think it's very likely that implementers would have some objects that need to be closed, and thus are leaking memory today, maybe we should actually just not default it at all, so they fix their memory leaks asap.

Conversely, if we think it's unlikely that the average implementer is going to leak memory, there's no reason we shouldn't just leave it defaulted forever.

We need the default implementation in any case until the next major release not to break existing code. Afterwards, I would be in favour of removing the default implementation, because then there is a bigger chance that existing memory leakages are found and future leakages are avoided.

Hm. On the one hand I'd guess not that many users in total are leaking memory here, but suspect that those who are advanced enough to be implementing ConfigSetter may likely be...

I'd agree we should remove the default in the next major release though, especially once we upgrade Rocks as some of the API changes since our current version effectively force creating more/new RocksObjects when setting options.

Might be worth to add a WARN log in the default implementation, telling users, that the default will be removed in 3.0.0 and they should overwrite it?

cadonna · 2019-05-08T11:41:19Z

+
+    /**
+     * This method will be called when the store is closed and should be used to close any user-constructed objects
+     * that inherit from RocksObject. Any such object created with new in setConfig should have close called on it


RocksObject -> {@code RocksObject}
setConfig -> {@link #setConfig}
close -> {@code close}

👍 Actually, I think @link would be preferable in all three cases, but they need to be properly qualified.

At first, I also thought links would be good, but then I thought that we also need to maintain those links to JavaDocs outside of Kafka? Do we have any policy about that?

I have no idea. I would assume that Gradle would take care of downloading the docs jar of the dependency and linking to the class appropriately. I think we'd actually have to build the javadoc to see if it works or not.

Ack, updated the javadocs -- can you take a quick look and see if everything seems reasonable?

cadonna · 2019-05-08T11:44:28Z

     */
    void setConfig(final String storeName, final Options options, final Map<String, Object> configs);
+
+    /**


Could you add a short one line description that will be shown in the Method Summary section of the JavaDocs?

vvcephei

LGTM, pending ongoing discussions. Thanks!

vvcephei · 2019-05-08T14:43:10Z

+
+    /**
+     * This method will be called when the store is closed and should be used to close any user-constructed objects
+     * that inherit from RocksObject. Any such object created with new in setConfig should have close called on it


👍 Actually, I think @link would be preferable in all three cases, but they need to be properly qualified.

vvcephei · 2019-05-08T14:43:39Z

+     * here to avoid leaking off-heap memory.
+     * Objects to be closed can be saved by the user or retrieved back from options using its getter methods.
+     *
+     * Example objects needing to be closed include Filter and Cache


same... @link > @close for this case.

vvcephei · 2019-05-08T14:47:07Z

+     * @param storeName     the name of the store being configured
+     * @param options       the Rocks DB options
+\    */
+    default void close(final String storeName, final Options options) {


That's a tricky question... The problem is that there's no way to signal ahead of time that you should override the method to avoid breakage when we remove default (unlike when you deprecate a method, which indicates you should make a change ahead of the breakage).

So, whenever we do make the change, it will be a breaking one.

If we think it's very likely that implementers would have some objects that need to be closed, and thus are leaking memory today, maybe we should actually just not default it at all, so they fix their memory leaks asap.

Conversely, if we think it's unlikely that the average implementer is going to leak memory, there's no reason we shouldn't just leave it defaulted forever.

cadonna · 2019-05-08T20:33:32Z

    void setConfig(final String storeName, final Options options, final Map<String, Object> configs);
+
+    /**
+     * This method should close any user-constructed objects that inherit from {@code org.rocksdb.RocksObject}


I would rather write the one line description in imperative.

Close any user-constructed objects that inherit from {@code org.rocksdb.RocksObject}.

cadonna · 2019-05-08T20:41:49Z

+    /**
+     * This method should close any user-constructed objects that inherit from {@code org.rocksdb.RocksObject}
+     *
+     * Any such object created with @{code new} in {@link #setConfig} should have {@code close} called on it here to


And then here

Any object created with @{code new} in {@link #setConfig} and that inherits from {@code org.rocksdb.RocksObject} should have {@code close()} called on it here to

Sounds good to me. Thanks for reviewing @cadonna !

Use @link and also link to should have {@link RocksObject#close() close()} called

Co-Authored-By: ableegoldman <ableegoldman@gmail.com>

mjsax · 2019-05-08T21:59:04Z

+
+    /**
+     * This method should close any user-constructed objects that inherit from {@code org.rocksdb.RocksObject}
+     *


nit: insert <p>

You don't need a closing
right? My IDE is inserting it automatically

Correct. My IDE does it, too... But single <p> is sufficient -- it's not strict HTML, but JavaDoc markup that looks like HTML

mjsax · 2019-05-08T22:00:39Z

+    /**
+     * This method should close any user-constructed objects that inherit from {@code org.rocksdb.RocksObject}
+     *
+     * Any such object created with @{code new} in {@link #setConfig} should have {@code close} called on it here to


Use @link and also link to should have {@link RocksObject#close() close()} called

mjsax · 2019-05-08T22:01:21Z

+     * Any such object created with @{code new} in {@link #setConfig} should have {@code close} called on it here to
+     * avoid leaking off-heap memory. Objects to be closed can be saved by the user or retrieved back from
+     * {@code options} using its getter methods.
+     *


nit: insert <p>

mjsax · 2019-05-08T22:01:37Z

+     * avoid leaking off-heap memory. Objects to be closed can be saved by the user or retrieved back from
+     * {@code options} using its getter methods.
+     *
+     * Example objects needing to be closed include {@code org.rocksdb.Filter} and {@code org.rocksdb.Cache}.


nit: as above, use @link

mjsax · 2019-05-08T22:01:57Z

+     * Example objects needing to be closed include {@code org.rocksdb.Filter} and {@code org.rocksdb.Cache}.
+     *
+     * @param storeName     the name of the store being configured
+     * @param options       the Rocks DB options


nit RocksDB without space

whoops, that was just copied from above. I'll fix it in #setConfig too

mjsax · 2019-05-08T22:06:16Z

+     * @param storeName     the name of the store being configured
+     * @param options       the Rocks DB options
+\    */
+    default void close(final String storeName, final Options options) {


Might be worth to add a WARN log in the default implementation, telling users, that the default will be removed in 3.0.0 and they should overwrite it?

cadonna · 2019-05-09T08:31:56Z

+     * Close any user-constructed objects that inherit from {@link org.rocksdb.RocksObject RocksObject}.
+     * <p>
+     * Any object created with @{code new} in {@link RocksDBConfigSetter#setConfig setConfig()} and that inherits
+     * from {@link org.rocksdb.RocksObject RocksObject} should have {@link org.rocksdb.RocksObject#close() close()}


Sorry for bringing this up again. I generated the JavaDocs with ./gradlew aggregatedJavadoc and there are no links generated for RocksObject and any other class outside Kafka. What we get is a non-fully qualified name in @code font, i.e., RocksObject. I would at least ensure that the fully-qualified name is displayed. See also the other occurrences of links to classes outside Kafka.

Ah, thanks for actually checking here...I meant to but "it worked in my IDE"...

Agreed that a fully qualified name here is important if we can't properly link though, I'll change these up yet again

Co-Authored-By: cadonna <bruno@confluent.io>

cadonna

Thanks @ableegoldman

ableegoldman · 2019-05-09T18:44:28Z

Thanks everyone for helping to straighten out the javadocs -- the KIP is now accepted so if everything looks sorted we can merge this when the build passes

guozhangwang

LGTM. We can merge it once builds are green.

bbejeck · 2019-05-09T22:45:46Z

Merged #6697 into trunk.

bbejeck · 2019-05-09T22:46:01Z

Thanks @ableegoldman!

…es-14-May * AK_REPO/trunk: (24 commits) KAFKA-7321: Add a Maximum Log Compaction Lag (KIP-354) (apache#6009) KAFKA-8335; Clean empty batches when sequence numbers are reused (apache#6715) KAFKA-6455: Session Aggregation should use window-end-time as record timestamp (apache#6645) KAFKA-6521: Use timestamped stores for KTables (apache#6667) [MINOR] Consolidate in-memory/rocksdb unit tests for window & session store (apache#6677) MINOR: Include StickyAssignor in system tests (apache#5223) KAFKA-7633: Allow Kafka Connect to access internal topics without cluster ACLs (apache#5918) MINOR: Align KTableAgg and KTableReduce (apache#6712) MINOR: Fix code section formatting in TROGDOR.md (apache#6720) MINOR: Remove unnecessary OptionParser#accepts method call from PreferredReplicaLeaderElectionCommand (apache#6710) KAFKA-8352 : Fix Connect System test failure 404 Not Found (apache#6713) KAFKA-8348: Fix KafkaStreams JavaDocs (apache#6707) MINOR: Add missing option for running vagrant-up.sh with AWS to vagrant/README.md KAFKA-8344; Fix vagrant-up.sh to work with AWS properly MINOR: docs typo in '--zookeeper myhost:2181--execute' MINOR: Remove header and key/value converter config value logging (apache#6660) KAFKA-8231: Expansion of ConnectClusterState interface (apache#6584) KAFKA-8324: Add close() method to RocksDBConfigSetter (apache#6697) KAFKA-6789; Handle retriable group errors in AdminClient API (apache#5578) KAFKA-8332: Refactor ImplicitLinkedHashSet to avoid losing ordering when converting to Scala ...

Following KIP-453, this PR adds a default close() method to the RocksDBConfigSetter interface and calls it when closing a store. Guozhang Wang <wangguoz@gmail.com>, Matthias J. Sax <mjsax@apache.org>, John Roesler <john@confluent.io>, Bruno Cadonna <bruno@confluent.io>

Added close method

df241b6

cadonna reviewed May 8, 2019

View reviewed changes

vvcephei approved these changes May 8, 2019

View reviewed changes

Updated javadocs

b139c6a

cadonna reviewed May 8, 2019

View reviewed changes

cadonna and others added 2 commits May 8, 2019 13:59

Missing period in javadoc

ca6ab3d

Co-Authored-By: ableegoldman <ableegoldman@gmail.com>

More edits to javadocs

d6c4ec6

mjsax added the streams label May 8, 2019

mjsax reviewed May 8, 2019

View reviewed changes

ableegoldman added 3 commits May 8, 2019 15:25

Github comments

f09100a

More javadocs fixes

ccf5027

removed unnecessary static and final qualifiers

f539316

cadonna reviewed May 9, 2019

View reviewed changes

ableegoldman and others added 2 commits May 9, 2019 11:25

Typo @{code} -> {@code}

7e3b7c5

Co-Authored-By: cadonna <bruno@confluent.io>

Revert javadoc @links back to @code

02779ae

cadonna approved these changes May 9, 2019

View reviewed changes

guozhangwang approved these changes May 9, 2019

View reviewed changes

bbejeck merged commit b2826c6 into apache:trunk May 9, 2019

mjsax added the kip Requires or implements a KIP label Jun 12, 2020

ableegoldman deleted the CloseRocksObjects branch June 26, 2020 22:38

lucasbru mentioned this pull request Dec 8, 2022

KAFKA-14432: RocksDBStore relies on finalizers to not leak memory #12935

Merged

3 tasks

Conversation

ableegoldman commented May 8, 2019

Uh oh!

ableegoldman commented May 8, 2019

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vvcephei 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!

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!