Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[fix] [broker] [branch-2.10] Upgrade rocksDB version to 6.16.4 to keep sync with BookKeeper 4.14.7 #20312

Conversation

hangc0276
Copy link
Contributor

@hangc0276 hangc0276 commented May 12, 2023

Fixes #xyz

Master Issue: #xyz

PIP: #xyz

Motivation

Pulsar 2.10 uses BookKeeper 4.14.7, and BookKeeper 4.14.7 uses RocksDB 6.16.4.
However, Pulsar also introduced RocksDB dependency and uses 6.10.2. When packaging all the dependencies into the Pulsar release package, the RocksDB 6.10.2 in Pulsar will override the RocksDB 6.16.4 in BookKeeper. The Pulsar release package will only contain RocksDB 6.10.2 in the end.

We encountered a RocksDB 6.10.2 memory leak issue when upgrading from Pulsar 2.9.x to 2.10.x, but still have not found any related issues in RocksDB. Pulsar 2.9.x uses RocksDB 6.16.4. One related issue is apache/bookkeeper#3507

The reason why we revert #20287 is that we found the bookie throws method not found exception when upgrading RocksDb to 6.29.4.1 #20243 (comment)

The root cause of the exception is

So we should upgrade the RocksDB version in Pulsar with the following steps.

  • Upgrade the RocksDB version to 6.16.4 to keep sync with BookKeeper 4.14.7 first to fix the RocksDB memory leak issue.
  • Upgrade BookKeeper branch-4.14 RocksDB version to 6.29.4.1 and trigger a new release 4.14.8
  • Upgrade Pulsar branch-2.10's BookKeeper dependency to 4.14.8
  • Upgrade Pulsar's RocksDB version to 6.29.4.1

The reason why we should upgrade Pulsar's RocksDB version to 6.29.4.1 is apache/bookkeeper#3734 (comment). Pulsar 3.0 uses BookKeeper 4.16.0 which uses RocksDB 7.9.2, and we should make sure Pulsar can roll back from 3.0 to 2.10.x

Modifications

Verifying this change

  • Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is a trivial rework / code cleanup without any test coverage.

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

  • Added integration tests for end-to-end deployment with large payloads (10MB)
  • Extended integration test for recovery after broker failure

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository:

@hangc0276 hangc0276 self-assigned this May 12, 2023
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label May 12, 2023
@hangc0276 hangc0276 added dependencies Pull requests that update a dependency file release/2.10.5 and removed doc-not-needed Your PR changes do not impact docs labels May 12, 2023
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label May 12, 2023
@hangc0276 hangc0276 changed the title Upgrade rocksDB version to 6.16.4 to keep sync with BookKeeper 4.14.7 [fix] [broker] [branch-2.10] Upgrade rocksDB version to 6.16.4 to keep sync with BookKeeper 4.14.7 May 12, 2023
@eolivelli
Copy link
Contributor

could you please fill in the description ?

why this patch is better than #20287 ?
how did you choose 6.16.4 ?

@hangc0276
Copy link
Contributor Author

could you please fill in the description ?

why this patch is better than #20287 ? how did you choose 6.16.4 ?

@eolivelli Thanks for your reminder. I updated the description, please help take a look, thanks.

@hangc0276
Copy link
Contributor Author

/pulsarbot run-failure-checks

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the clarification

+1

Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lhotari
Copy link
Member

lhotari commented May 12, 2023

The python tests seem to break because of an unrelated Python 2.7 compatibility issues

======================================================================
ERROR: test_python_instance (unittest.loader.ModuleImportFailure)
----------------------------------------------------------------------
ImportError: Failed to import test module: test_python_instance
Traceback (most recent call last):
  File "/usr/lib/python2.7/unittest/loader.py", line 254, in _find_tests
    module = self._get_module_from_name(name)
  File "/usr/lib/python2.7/unittest/loader.py", line 232, in _get_module_from_name
    __import__(name)
  File "/pulsar/pulsar-functions/instance/target/python-instance/tests/test_python_instance.py", line 28, in <module>
    from python_instance import InstanceConfig
  File "/pulsar/pulsar-functions/instance/target/python-instance/python_instance.py", line 44, in <module>
    import state_context
  File "/pulsar/pulsar-functions/instance/target/python-instance/state_context.py", line 26, in <module>
    from bookkeeper import admin, kv
  File "/usr/local/lib/python2.7/dist-packages/bookkeeper/admin/__init__.py", line 15, in <module>
    from bookkeeper.admin.client import Client
  File "/usr/local/lib/python2.7/dist-packages/bookkeeper/admin/client.py", line 20, in <module>
    from bookkeeper import types
  File "/usr/local/lib/python2.7/dist-packages/bookkeeper/types.py", line 27, in <module>
    from bookkeeper.common.protobuf_helpers import get_messages
  File "/usr/local/lib/python2.7/dist-packages/bookkeeper/common/protobuf_helpers.py", line 18, in <module>
    from collections.abc import Mapping
ImportError: No module named abc

it seems to be related to apache/bookkeeper#3875 changes.

@zymap do you have a solution for fixing the Python tests in branch-2.10?

@lhotari lhotari merged commit 1c00154 into apache:branch-2.10 May 12, 2023
30 of 32 checks passed
@lhotari
Copy link
Member

lhotari commented May 12, 2023

I created a separate issue #20314 for dealing with the broken Python tests in branch-2.10

lhotari pushed a commit to datastax/pulsar that referenced this pull request May 12, 2023
…p sync with BookKeeper 4.14.7 (apache#20312)

(cherry picked from commit 1c00154)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-picked/branch-2.10 dependencies Pull requests that update a dependency file doc-not-needed Your PR changes do not impact docs release/2.10.5
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants