Make the rocksDB configuration compatible with previous versions #3523

zymap · 2022-10-12T02:41:14Z

Motivation

In PR #3056, we introduced using rocksDB configuration file to configure it. But it is not compatible with the previous versions of bookkeeper. We used to use the bookie configuration file to configure it. It would make the existing configuration doesn't work. Which causes some performance issues for the user's existing environment.

This PR adds the old configuration back. The new way only works if the rocksDB configuration file is existing.

--- *Motivation* In the PR 3056, we introduced using rocksDB configuration file to configure it. But it is not compatible with the previous versions of bookkeeper. We used to use the bookie configuration file to configure it. It would make the existing configuration doesn't work. Which causes some performance issues for the user's existing environment. This PR adds the old configuration back. The new way only works if the rocksDB configuration file is existing.

zymap · 2022-10-12T02:56:18Z

I am trying to add a test to make sure it uses the right configuration.

eolivelli

I support this patch.
I left one comment.

also we need to add some tests

...er-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/KeyValueStorageRocksDB.java

zymap · 2022-10-13T02:19:03Z

@eolivelli Added tests and logs. PTAL

StevenLuMT

I have a question why It would make the existing configuration doesn't work?
which config key causes some performance issues for the user's existing environment?

zymap · 2022-10-13T03:01:59Z

@StevenLuMT We were using bk_server.conf to configure the rocksDB before, this change will make the existing configuration in bk_server.conf invalid.
Please see the discussion in the mailing list: https://lists.apache.org/thread/drh4p5prxbcs8gszhxnd1xsv0g48vvbt

StevenLuMT · 2022-10-13T11:02:03Z

@StevenLuMT We were using bk_server.conf to configure the rocksDB before, this change will make the existing configuration in bk_server.conf invalid. Please see the discussion in the mailing list: https://lists.apache.org/thread/drh4p5prxbcs8gszhxnd1xsv0g48vvbt

have a look this mail reply, I think the implementation of the compatible scheme can have more @zymap @eolivelli
https://lists.apache.org/thread/xxtx9nsz07rywfbgw6rp15ljh6sw5jyw

zymap · 2022-10-13T12:16:55Z

have a look this mail reply, I think the implementation of the compatible scheme can have more

@StevenLuMT Could you please talk more about this?

I think I just want to bring the original way back. Let it doesn't affect any user who is using the bk_server.conf or entry_location_rocksDB.conf.

zymap · 2022-10-14T01:30:52Z

@eolivelli @StevenLuMT please take another look when you have time. Thanks

eolivelli

LGTM

nicoloboschi

Great work and great catch!

StevenLuMT

I think the implementation of the compatible scheme can have more
https://lists.apache.org/thread/xxtx9nsz07rywfbgw6rp15ljh6sw5jyw
we can merge this first, and I will mention a pr later to make this thing better

zymap · 2022-10-17T07:54:48Z

Thank you, guys!

* Make the rocksDB configuration compatible with previous versions --- *Motivation* In the PR 3056, we introduced using rocksDB configuration file to configure it. But it is not compatible with the previous versions of bookkeeper. We used to use the bookie configuration file to configure it. It would make the existing configuration doesn't work. Which causes some performance issues for the user's existing environment. This PR adds the old configuration back. The new way only works if the rocksDB configuration file is existing. * Add tests and logs (cherry picked from commit e313f60)

--- *Motivation* Related PR: apache#3523 Mailing discussion: https://lists.apache.org/thread/drh4p5prxbcs8gszhxnd1xsv0g48vvbt We introduce the new way to configure the RocksDB but we haven't mentioned it in the website and release-note. This PR add the document for the RocksDB configuration updates. And rename the RocksDB configuration files with the suffix `.default` to avoid loading them by default. *Modification* - Add the configuration changes in the 4.15.0 release notes. - Add how to configure the RocksDB with the new file in the website. - Rename the file with `.default` suffix to avoid loading it by default.

hangc0276 · 2022-10-17T03:56:11Z

...er-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/KeyValueStorageRocksDB.java


+
 import static com.google.common.base.Preconditions.checkState;
+//CHECKSTYLE.OFF: IllegalImport


Why add the checkstyle off?

The checkstyle doesn't allow we use this imports io.netty.util.internal.PlatformDependent.maxDirectMemory. We disabled it before https://github.com/apache/bookkeeper/pull/3056/files#diff-cbb728e9ff8fae16673eb48455d2eebc18d12ed8fec6a93266bb9363527dad97L25

hangc0276 · 2022-10-17T08:48:46Z

...er-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/KeyValueStorageRocksDB.java

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


Why do we need this options field?

It's just for testing. I need to get configurations from the option to test if it is loading the right configuration.

hangc0276 · 2022-10-17T08:53:56Z

In BookKeeper and Pulsar's conf folder, it already has default RocksDB configuration files. In this Pr, the loading order is RocksDB Configuration file > conf/bk_server.conf, which means it will still load the RocksDB configuration file by default and will cause a compatible issue in the BookKeeper version upgrade.

zymap · 2022-10-17T09:01:40Z

@hangc0276 We rename the configuration file in this PR #3540

--- *Motivation* Related PR: #3523 Mailing discussion: https://lists.apache.org/thread/drh4p5prxbcs8gszhxnd1xsv0g48vvbt We introduce the new way to configure the RocksDB but we haven't mentioned it in the website and release-note. This PR add the document for the RocksDB configuration updates. And rename the RocksDB configuration files with the suffix `.default` to avoid loading them by default. *Modification* - Add the configuration changes in the 4.15.0 release notes. - Add how to configure the RocksDB with the new file in the website. - Rename the file with `.default` suffix to avoid loading it by default.

--- *Motivation* Related PR: #3523 Mailing discussion: https://lists.apache.org/thread/drh4p5prxbcs8gszhxnd1xsv0g48vvbt We introduce the new way to configure the RocksDB but we haven't mentioned it in the website and release-note. This PR add the document for the RocksDB configuration updates. And rename the RocksDB configuration files with the suffix `.default` to avoid loading them by default. *Modification* - Add the configuration changes in the 4.15.0 release notes. - Add how to configure the RocksDB with the new file in the website. - Rename the file with `.default` suffix to avoid loading it by default. (cherry picked from commit 1966512)

…che#3523) * Make the rocksDB configuration compatible with previous versions --- *Motivation* In the PR 3056, we introduced using rocksDB configuration file to configure it. But it is not compatible with the previous versions of bookkeeper. We used to use the bookie configuration file to configure it. It would make the existing configuration doesn't work. Which causes some performance issues for the user's existing environment. This PR adds the old configuration back. The new way only works if the rocksDB configuration file is existing. * Add tests and logs (cherry picked from commit e313f60)

…che#3523) * Make the rocksDB configuration compatible with previous versions --- *Motivation* In the PR 3056, we introduced using rocksDB configuration file to configure it. But it is not compatible with the previous versions of bookkeeper. We used to use the bookie configuration file to configure it. It would make the existing configuration doesn't work. Which causes some performance issues for the user's existing environment. This PR adds the old configuration back. The new way only works if the rocksDB configuration file is existing. * Add tests and logs

--- *Motivation* Related PR: apache#3523 Mailing discussion: https://lists.apache.org/thread/drh4p5prxbcs8gszhxnd1xsv0g48vvbt We introduce the new way to configure the RocksDB but we haven't mentioned it in the website and release-note. This PR add the document for the RocksDB configuration updates. And rename the RocksDB configuration files with the suffix `.default` to avoid loading them by default. *Modification* - Add the configuration changes in the 4.15.0 release notes. - Add how to configure the RocksDB with the new file in the website. - Rename the file with `.default` suffix to avoid loading it by default.

zymap added the release/4.15.3 label Oct 12, 2022

zymap added this to the 4.16.0 milestone Oct 12, 2022

zymap requested review from StevenLuMT, eolivelli and hangc0276 October 12, 2022 02:41

zymap self-assigned this Oct 12, 2022

eolivelli requested changes Oct 12, 2022

View reviewed changes

Add tests and logs

4bcef17

StevenLuMT requested changes Oct 13, 2022

View reviewed changes

merlimat approved these changes Oct 14, 2022

View reviewed changes

zymap requested review from StevenLuMT and eolivelli October 14, 2022 02:00

eolivelli approved these changes Oct 17, 2022

View reviewed changes

nicoloboschi approved these changes Oct 17, 2022

View reviewed changes

StevenLuMT approved these changes Oct 17, 2022

View reviewed changes

nicoloboschi merged commit e313f60 into apache:master Oct 17, 2022

nicoloboschi added the cherry-picked/branch-4.15 label Oct 17, 2022

zymap mentioned this pull request Oct 17, 2022

Update website to record the RocksDB configuration changes #3540

Merged

hangc0276 reviewed Oct 17, 2022

View reviewed changes

This was referenced Mar 3, 2025

Restore support for dbStorage_rocksDB_* settings defined in bookkeeper.configData apache/pulsar-helm-chart#580

Merged

Revisit solution to configure Bookkeeper RocksDB settings using individual RocksDB config files apache/pulsar-helm-chart#583

Merged



		import static com.google.common.base.Preconditions.checkState;
		//CHECKSTYLE.OFF: IllegalImport

Make the rocksDB configuration compatible with previous versions #3523

Make the rocksDB configuration compatible with previous versions #3523

Uh oh!

Conversation

zymap commented Oct 12, 2022

Motivation

Uh oh!

zymap commented Oct 12, 2022

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zymap commented Oct 13, 2022

Uh oh!

StevenLuMT left a comment

Choose a reason for hiding this comment

Uh oh!

zymap commented Oct 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

StevenLuMT commented Oct 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zymap commented Oct 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zymap commented Oct 14, 2022

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

nicoloboschi left a comment

Choose a reason for hiding this comment

Uh oh!

StevenLuMT left a comment

Choose a reason for hiding this comment

Uh oh!

zymap commented Oct 17, 2022

Uh oh!

hangc0276 Oct 17, 2022

Choose a reason for hiding this comment

Uh oh!

zymap Oct 17, 2022

Choose a reason for hiding this comment

Uh oh!

hangc0276 Oct 17, 2022

Choose a reason for hiding this comment

Uh oh!

zymap Oct 17, 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!

hangc0276 commented Oct 17, 2022

Uh oh!

zymap commented Oct 17, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

zymap commented Oct 13, 2022 •

edited

Loading

StevenLuMT commented Oct 13, 2022 •

edited

Loading

zymap commented Oct 13, 2022 •

edited

Loading

zymap Oct 17, 2022 •

edited

Loading