KAFKA-15443: Upgrade RocksDB to 9.7.3 #18275

swikarpat · 2024-12-19T18:47:44Z

Upgrades RocksDB to version 9.7.3 to address Jira KAFKA-15443

This PR addresses the following compatibility issues introduced by the RocksDB upgrade:

Removal of AccessHint: The AccessHint class was completely removed in RocksDB 9.7.3. This required removing all import statements, variable declarations, method parameters, method return types, and static method calls related to AccessHint in RocksDBGenericOptionsToDbOptionsColumnFamilyOptionsAdapter.java RocksDBGenericOptionsToDbOptionsColumnFamilyOptionsAdapterTest.java Unused methods are removed in RocksDBGenericOptionsToDbOptionsColumnFamilyOptionsAdapter.java
Removal of NO_FILE_CLOSES: The NO_FILE_CLOSES metric was also removed in RocksDB 9.7.3. The calculation for numberOfOpenFiles in RocksDBMetricsRecorder.java has been adjusted to now track the total number of file opens since the last reset. The previous calculation, which subtracted NO_FILE_CLOSES from NO_FILE_OPENS, is no longer possible.
The reason RocksDB team removed NO_FILE_CLOSES it seems to me they detected not properly working: https://github.com/search?q=repo%3Afacebook%2Frocksdb+NO_FILE_CLOSES&type=issues

Any feedback or any proposed changes you would like me to do are welcome let me know.
@cadonna @mjsax

Committer Checklist (excluded from commit message)

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

mjsax

Thanks for the PR. I think we need to bite the bullet and merge this as-is. Given that we are heading into a major release, it should hopefully be ok.

Curious to hear how others think about it. \cc @cadonna @ableegoldman

mjsax · 2024-12-20T03:47:13Z

...s/src/main/java/org/apache/kafka/streams/state/internals/metrics/RocksDBMetricsRecorder.java

+
+            // NO_FILE_CLOSES metric was removed in RocksDB 9.7.3.
+            // This now tracks the total number of file opens since the last reset.
+            numberOfOpenFiles += valueProviders.statistics.getAndResetTickerCount(TickerType.NO_FILE_OPENS);


As commented on the Jira ticket, changing semantics of an existing metric does not sound right. I would rather just report -1 surrogate for now, and deprecate this metric with 4.0 release.

Curious to hear what others think.

Tend to agree that changing semantics is no good, but it sounds like the new metric will be useful -- is it possible to deprecate the old one and introduce a new one that corresponds to the same underlying thing? I really don't know anything about how we expose the RocksDB metrics though so Bruno is probably better suited to chime in

@mjsax Thanks for the suggestion. I'm not entirely sure how to implement reporting a -1 surrogate and deprecating the metric for the 4.0 release. Could you provide some guidance or point me to relevant examples? NO_FILE_CLOSES metric declared in TickerType.class inside org.rocksdb package. We can't modify that class, can we? @cadonna @ableegoldman

@swikarpat I believe @mjsax means to not return the number of opened files but just -1. I agree with him. The number of opened files without the number of closed files does not give us much info. Furthermore, by recording the number of opened files, the metric would seem correct, but actually would not show the correct values because the metric is defined as the number of currently open files. Returning constant -1 would make it clear that the metric does not work.
In addition to returning -1, could you please also update the documentation of the metrics in this PR:

kafka/docs/ops.html

Line 3457 in a0a5019

<td>number-open-files</td>

mjsax · 2024-12-20T03:48:20Z

...afka/streams/state/internals/RocksDBGenericOptionsToDbOptionsColumnFamilyOptionsAdapter.java

@@ -842,12 +830,6 @@ public Options setSstFileManager(final SstFileManager sstFileManager) {
        return this;
    }

-    @Override
-    public Options setLogger(final org.rocksdb.Logger logger) {
-        dbOptions.setLogger(logger);


If this was removed, too, can you add it to the PR description.

@mjsax thanks for feedback. I wanted to confirm setLogger() method. Parameter inside setLogger() has changed to LoggerInterface. I've made corrections and put method back in that java class.

It is fine to change it like you did. If users use a logger class that is derived from Logger they have to modify it in any case because Logger now implements interface LoggerInterface. Thus, the change of the method will most likely not directly affect users on a source level. It will affect them on binary level, though, but that is the trade-off here.

mjsax · 2024-12-20T03:52:08Z

...afka/streams/state/internals/RocksDBGenericOptionsToDbOptionsColumnFamilyOptionsAdapter.java

-
-    @Override
-    public AccessHint accessHintOnCompactionStart() {
-        return dbOptions.accessHintOnCompactionStart();


It's too bad that RockDB does not mark methods as deprecated but just removes stuff.

This was always an issue for us (even if we have to admit, that we are upgrade from a quite old version...). I guess we just need to bite the bullet and make this breaking change... Seem we need to start tracking RocksDB changes more closely and frequently and do KIPs early on to prepare for breaking changes. (What basically implies, that we can upgrade RocksDB only in major release if there are breaking changes...)

Or we change our "policy" for RocksDB compatibility and document it properly, reserving the right to break compatibility via RocksDB version bumps also in minor release.

Yeah this drives me crazy. Personally I think it's ok to bite the bullet on this one but let's definitely come up with a strategy for this going forward. I'd say a good compromise is to meet in the middle and do both, that is, publish an official policy for RocksDB APIs which says there is no guarantee of the usual one-year deprecation & only major version removal, and any deprecations are best-effort. I'm less worried about the deprecations and more about restricting our ability to upgrade rocksdb to only major releases.

At the same time, I think we can and should do better. I like the idea of looking ahead at each new RocksDB release to mark any changed or removed APIs exposed to our users so we can deprecate them ASAP. We can hash out the details of how often and how to share this responsibility later (for example maybe this should be one of the Streams RM responsibilities, to look for upcoming API changes in future rocks and make sure they get deprecated in that release

cadonna

Thanks for the PR @swikarpat !

Besides methods that were removed, there is at least one method that was added, i.e., setMemtableMaxRangeDeletions(). That is also the reason why RocksDBGenericOptionsToDbOptionsColumnFamilyOptionsAdapterTest fails.

In the compatibility report you attached to the ticket there are quite some compatibility issues listed. IMO, we should state the most important ones on the upgrade notes and link to the compatibility report for details. Could you please update the upgrade notes with this info in this PR?

I left some more comments on the PR.

cadonna · 2024-12-20T08:42:13Z

...s/src/main/java/org/apache/kafka/streams/state/internals/metrics/RocksDBMetricsRecorder.java

+
+            // NO_FILE_CLOSES metric was removed in RocksDB 9.7.3.
+            // This now tracks the total number of file opens since the last reset.
+            numberOfOpenFiles += valueProviders.statistics.getAndResetTickerCount(TickerType.NO_FILE_OPENS);


@swikarpat I believe @mjsax means to not return the number of opened files but just -1. I agree with him. The number of opened files without the number of closed files does not give us much info. Furthermore, by recording the number of opened files, the metric would seem correct, but actually would not show the correct values because the metric is defined as the number of currently open files. Returning constant -1 would make it clear that the metric does not work.
In addition to returning -1, could you please also update the documentation of the metrics in this PR:

kafka/docs/ops.html

Line 3457 in a0a5019

<td>number-open-files</td>

cadonna · 2024-12-20T08:52:07Z

...afka/streams/state/internals/RocksDBGenericOptionsToDbOptionsColumnFamilyOptionsAdapter.java

@@ -842,12 +830,6 @@ public Options setSstFileManager(final SstFileManager sstFileManager) {
        return this;
    }

-    @Override
-    public Options setLogger(final org.rocksdb.Logger logger) {
-        dbOptions.setLogger(logger);


It is fine to change it like you did. If users use a logger class that is derived from Logger they have to modify it in any case because Logger now implements interface LoggerInterface. Thus, the change of the method will most likely not directly affect users on a source level. It will affect them on binary level, though, but that is the trade-off here.

swikarpat · 2024-12-22T02:27:25Z

@cadonna @mjsax I have added methods and also made changes in Test class and also documents. Unit Tests keep failing. Though I ran unit tests for given two Test file and ran passed. Can you please take a look what went wrong and feedback?

Total 8 files changed.

ops.html
upgrade-guide.html
dependencies.gradle
RocksDBGenericOptionsToDbOptionsColumnFamilyOptionsAdapter.java
RocksDBMetricsRecorder.java
RocksDBGenericOptionsToDbOptionsColumnFamilyOptionsAdapterTest.java
RocksDBMetricsRecorderTest.java

Thanks

mjsax · 2025-01-02T21:25:31Z

RocksDB did push out newer release since we started this PR. There is now 9.10.0 -- not sure if we want to go with a .0 version, but there is also 9.9.3. -- Should we use the latest version instead of 9.7.3?

mjsax · 2025-01-02T21:27:18Z

Unit Tests keep failing. Though I ran unit tests for given two Test file and ran passed. Can you please take a look what went wrong and feedback?

No need to worry about these. We know they are flaky, and AFAIK somebody is already working on a fix. As long as no test related to this PR fail, we can merge anyway. Btw: in case a test get's fixed, you would need to rebase your branch to trunk to pickup the fix.

mjsax · 2025-01-02T21:28:38Z

docs/streams/upgrade-guide.html

@@ -139,8 +139,6 @@ <h3 class="anchor-heading"><a id="streams_notable_changes" class="anchor-link"><
        More details about the new config <code>StreamsConfig#TOPOLOGY_OPTIMIZATION_CONFIG</code> can be found in <a href="https://cwiki.apache.org/confluence/display/KAFKA/KIP-295%3A+Add+Streams+Configuration+Allowing+for+Optional+Topology+Optimization">KIP-295</a>.
    </p>

-    <h3><a id="streams_api_changes_400" href="#streams_api_changes_400">Streams API changes in 4.0.0</a></h3>


Why did you move this line?

Still not clear why this line was moved.

docs/streams/upgrade-guide.html

...afka/streams/state/internals/RocksDBGenericOptionsToDbOptionsColumnFamilyOptionsAdapter.java

.../streams/state/internals/RocksDBGenericOptionsToDbOptionsColumnFamilyOptionsAdapterTest.java

swikarpat · 2025-01-02T23:58:31Z

RocksDB did push out newer release since we started this PR. There is now 9.10.0 -- not sure if we want to go with a .0 version, but there is also 9.9.3. -- Should we use the latest version instead of 9.7.3?

@mjsax Thanks for feedback. Yes I am aware of that but only up to 9.7.3 currently published on maven central. I've asked RocksDB team for timeline of publishing latest version (9.10.0 released today) on maven central as here facebook/rocksdb#13214

mjsax

LGTM.

swikarpat · 2025-01-04T04:06:25Z

@mjsax Now I have rebased onto the latest trunk. Could you please merge this PR as I do not have write access to merge. Thank you

cadonna

Thanks for the updates, @swikarpat !

I have a couple of comments.

docs/streams/upgrade-guide.html

cadonna · 2025-01-06T10:22:45Z

docs/ops.html

-    <tr>
-      <td>number-open-files</td>
-      <td>The number of current open files.</td>
-      <td>kafka.streams:type=stream-state-metrics,thread-id=([-.\w]+),task-id=([-.\w]+),[store-scope]-id=([-.\w]+)</td>
-    </tr>


Instead of removing this row, could you add a comment that the metric will return constant -1 and why it returns that?
In the end the metric is still exposed, it just returns a useless constant value.

If you want, you are welcome to create a KIP to remove this metric from Streams. Once that KIP is accepted, we can remove the metric from in the next major release.

docs/streams/upgrade-guide.html

cadonna · 2025-01-06T10:37:28Z

docs/streams/upgrade-guide.html

@@ -139,8 +139,6 @@ <h3 class="anchor-heading"><a id="streams_notable_changes" class="anchor-link"><
        More details about the new config <code>StreamsConfig#TOPOLOGY_OPTIMIZATION_CONFIG</code> can be found in <a href="https://cwiki.apache.org/confluence/display/KAFKA/KIP-295%3A+Add+Streams+Configuration+Allowing+for+Optional+Topology+Optimization">KIP-295</a>.
    </p>

-    <h3><a id="streams_api_changes_400" href="#streams_api_changes_400">Streams API changes in 4.0.0</a></h3>


Still not clear why this line was moved.

docs/streams/upgrade-guide.html

...afka/streams/state/internals/RocksDBGenericOptionsToDbOptionsColumnFamilyOptionsAdapter.java

swikarpat · 2025-01-06T22:23:25Z

@cadonna Thanks for your feedback. I've made changes accordingly:
Removed deprecated methods, added row back to ops.html, made corrections to upgrade-guide.html.
Let me know if anything .. Thanks.

cadonna

Thanks for the updates, @swikarpat !

I have a couple of minor comments!

docs/ops.html

docs/streams/upgrade-guide.html

swikarpat · 2025-01-07T18:28:26Z

@cadonna Thanks for feedback. I've adjusted your suggested changes accordingly.

This PR addresses the following compatibility issues introduced by the RocksDB upgrade: Removal of AccessHint: The AccessHint class was completely removed in RocksDB 9.7.3. This required removing all import statements, variable declarations, method parameters, method return types, and static method calls related to AccessHint in RocksDBGenericOptionsToDbOptionsColumnFamilyOptionsAdapter.java RocksDBGenericOptionsToDbOptionsColumnFamilyOptionsAdapterTest.java Unused methods are removed in RocksDBGenericOptionsToDbOptionsColumnFamilyOptionsAdapter.java Removal of NO_FILE_CLOSES: The NO_FILE_CLOSES metric was also removed in RocksDB 9.7.3. The calculation for numberOfOpenFiles in RocksDBMetricsRecorder.java has been adjusted to now track the total number of file opens since the last reset. The previous calculation, which subtracted NO_FILE_CLOSES from NO_FILE_OPENS, is no longer possible. The reason RocksDB team removed NO_FILE_CLOSES it seems to me they detected not properly working: https://github.com/search?q=repo%3Afacebook%2Frocksdb+NO_FILE_CLOSES&type=issues KAFKA-15443: Upgrade RocksDB to 9.7.3 Update docs/ops.html Co-authored-by: Bruno Cadonna <[email protected]> Update docs/streams/upgrade-guide.html Co-authored-by: Bruno Cadonna <[email protected]> Update docs/streams/upgrade-guide.html Co-authored-by: Bruno Cadonna <[email protected]> Update upgrade-guide.html

cadonna

Thanks for the updates, @swikarpat !

LGTM!

I restarted the failed job. Once that terminates, I am going to merge this PR.

github-actions bot added triage PRs from the community streams dependencies Pull requests that update a dependency file build Gradle build or GitHub Actions small Small PRs labels Dec 19, 2024

swikarpat changed the title ~~Upgrade RocksDB to 9.7.3~~ KAFKA-15443 Upgrade RocksDB to 9.7.3 Dec 19, 2024

mjsax reviewed Dec 20, 2024

View reviewed changes

ableegoldman added the ci-approved label Dec 20, 2024

cadonna reviewed Dec 20, 2024

View reviewed changes

github-actions bot removed the triage PRs from the community label Dec 21, 2024

swikarpat requested review from mjsax, cadonna and ableegoldman December 22, 2024 02:27

mjsax reviewed Jan 2, 2025

View reviewed changes

docs/streams/upgrade-guide.html Outdated Show resolved Hide resolved

mjsax reviewed Jan 2, 2025

View reviewed changes

docs/streams/upgrade-guide.html Outdated Show resolved Hide resolved

mjsax reviewed Jan 2, 2025

View reviewed changes

...afka/streams/state/internals/RocksDBGenericOptionsToDbOptionsColumnFamilyOptionsAdapter.java Outdated Show resolved Hide resolved

mjsax reviewed Jan 2, 2025

View reviewed changes

.../streams/state/internals/RocksDBGenericOptionsToDbOptionsColumnFamilyOptionsAdapterTest.java Outdated Show resolved Hide resolved

swikarpat requested a review from mjsax January 3, 2025 00:00

mjsax approved these changes Jan 3, 2025

View reviewed changes

swikarpat force-pushed the upgrade-rocksdb-to-9.7.3 branch from f0dd7fd to 1af566b Compare January 3, 2025 20:29

swikarpat requested a review from mjsax January 3, 2025 20:35

swikarpat force-pushed the upgrade-rocksdb-to-9.7.3 branch 4 times, most recently from 20b16a4 to 3c43871 Compare January 3, 2025 23:33

swikarpat force-pushed the upgrade-rocksdb-to-9.7.3 branch from 3c43871 to 124ef06 Compare January 4, 2025 03:59

cadonna changed the title ~~KAFKA-15443 Upgrade RocksDB to 9.7.3~~ KAFKA-15443: Upgrade RocksDB to 9.7.3 Jan 6, 2025

cadonna reviewed Jan 6, 2025

View reviewed changes

github-actions bot removed the small Small PRs label Jan 6, 2025

swikarpat requested a review from cadonna January 6, 2025 22:21

cadonna reviewed Jan 7, 2025

View reviewed changes

docs/ops.html Outdated Show resolved Hide resolved

docs/streams/upgrade-guide.html Outdated Show resolved Hide resolved

docs/streams/upgrade-guide.html Outdated Show resolved Hide resolved

docs/streams/upgrade-guide.html Outdated Show resolved Hide resolved

swikarpat requested a review from cadonna January 7, 2025 18:27

swikarpat force-pushed the upgrade-rocksdb-to-9.7.3 branch 6 times, most recently from 3a4c2b7 to 01a7aaf Compare January 8, 2025 22:08

swikarpat force-pushed the upgrade-rocksdb-to-9.7.3 branch from 01a7aaf to 6ae30ed Compare January 9, 2025 01:24

cadonna approved these changes Jan 9, 2025

View reviewed changes

cadonna merged commit 8b72204 into apache:trunk Jan 9, 2025
9 checks passed

swikarpat deleted the upgrade-rocksdb-to-9.7.3 branch January 9, 2025 15:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-15443: Upgrade RocksDB to 9.7.3 #18275

KAFKA-15443: Upgrade RocksDB to 9.7.3 #18275

swikarpat commented Dec 19, 2024 •

edited

Loading

mjsax left a comment

mjsax Dec 20, 2024

ableegoldman Dec 20, 2024

swikarpat Dec 20, 2024 •

edited

Loading

cadonna Dec 20, 2024

mjsax Dec 20, 2024

swikarpat Dec 20, 2024 •

edited

Loading

cadonna Dec 20, 2024

mjsax Dec 20, 2024

ableegoldman Dec 20, 2024

cadonna left a comment

cadonna Dec 20, 2024

cadonna Dec 20, 2024

swikarpat commented Dec 22, 2024 •

edited

Loading

mjsax commented Jan 2, 2025

mjsax commented Jan 2, 2025

mjsax Jan 2, 2025

cadonna Jan 6, 2025

swikarpat commented Jan 2, 2025 •

edited

Loading

mjsax left a comment

swikarpat commented Jan 4, 2025 •

edited

Loading

cadonna left a comment

cadonna Jan 6, 2025

cadonna Jan 6, 2025

swikarpat commented Jan 6, 2025 •

edited

Loading

cadonna left a comment

swikarpat commented Jan 7, 2025

cadonna left a comment •

edited

Loading

KAFKA-15443: Upgrade RocksDB to 9.7.3 #18275

KAFKA-15443: Upgrade RocksDB to 9.7.3 #18275

Conversation

swikarpat commented Dec 19, 2024 • edited Loading

Committer Checklist (excluded from commit message)

mjsax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swikarpat Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swikarpat Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cadonna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swikarpat commented Dec 22, 2024 • edited Loading

mjsax commented Jan 2, 2025

mjsax commented Jan 2, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swikarpat commented Jan 2, 2025 • edited Loading

mjsax left a comment

Choose a reason for hiding this comment

swikarpat commented Jan 4, 2025 • edited Loading

cadonna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swikarpat commented Jan 6, 2025 • edited Loading

cadonna left a comment

Choose a reason for hiding this comment

swikarpat commented Jan 7, 2025

cadonna left a comment • edited Loading

Choose a reason for hiding this comment

swikarpat commented Dec 19, 2024 •

edited

Loading

swikarpat Dec 20, 2024 •

edited

Loading

swikarpat Dec 20, 2024 •

edited

Loading

swikarpat commented Dec 22, 2024 •

edited

Loading

swikarpat commented Jan 2, 2025 •

edited

Loading

swikarpat commented Jan 4, 2025 •

edited

Loading

swikarpat commented Jan 6, 2025 •

edited

Loading

cadonna left a comment •

edited

Loading