Skip to content

Conversation

@yangl
Copy link
Contributor

@yangl yangl commented Feb 1, 2023

Fixes #18834 #18672

Motivation

When run the separate meta store between the Pulsar broker cluster and the Bookkeeper cluster, the metadataStoreUrl (broker .conf) different from the metadataServiceUri (bookkeeper.conf), the auto recovery (standlone) cannot get the right rack info because when we set the rack info by admin bookies set-bookie-rack, the rack info on the broker side, so we should get the rack info from the pulsar broker side, not the bookkeeper side.

Modifications

First get from the broker side,then downgrade to the old logic(bk side )

- String metadataServiceUri = ConfigurationStringUtil.castToString(conf.getProperty("metadataServiceUri"));
+ String metadataServiceUri = ConfigurationStringUtil.castToString(conf.getProperty("metadataStoreUrl"));

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: yangl#19

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Feb 1, 2023
@yangl yangl changed the title [bug] fix the autoRecovery aware of the right rack info. [fix][broker] fix the autoRecovery aware of the right rack info. Feb 1, 2023
@yangl yangl force-pushed the autorecovery_rack branch from cafe0fd to 266de03 Compare February 1, 2023 07:47
@yangl yangl force-pushed the autorecovery_rack branch from 9993ba3 to a49b545 Compare February 2, 2023 02:49
Copy link
Member

@horizonzy horizonzy left a comment

Choose a reason for hiding this comment

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

I recommend setting rack info in the bk zookeeper cluster.

@yangl
Copy link
Contributor Author

yangl commented Feb 6, 2023

I recommend setting rack info in the bk zookeeper cluster.

Of course, it would be best to migrate the Metadata BookieRackAffinityMapping strategy to the bookkeeper project so that bookkeeper-only businesses can have this capability as well.

@horizonzy
Copy link
Member

cc @hangc0276

String metadataServiceUri = ConfigurationStringUtil.castToString(conf.getProperty("metadataServiceUri"));
if (StringUtils.isNotBlank(metadataServiceUri)) {
// First get from the Pulsar broker side
String metadataStoreUrl = ConfigurationStringUtil.castToString(conf.getProperty("metadataStoreUrl"));
Copy link
Contributor

Choose a reason for hiding this comment

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

This logic is used for bk auto-recovery, and it doesn't make sense to get it from the Pulsar broker side. If we use it with this logic, we need to configure metadataStoreUrl in the conf/bookkeeper.conf

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current BookieRackAffinityMapping policy is implemented on the pulsar side and its configuration is configured to the pulsar's zk (metadatastore).
It would be best to migrate the Metadata BookieRackAffinityMapping strategy to the bookkeeper project so that bookkeeper-only businesses can have this capability as well.

@github-actions
Copy link

github-actions bot commented Apr 3, 2023

The pr had no activity for 30 days, mark with Stale label.

@github-actions github-actions bot added the Stale label Apr 3, 2023
@Technoboy- Technoboy- added this to the 3.2.0 milestone Jul 31, 2023
@Technoboy- Technoboy- modified the milestones: 3.2.0, 3.3.0 Dec 22, 2023
@coderzc coderzc modified the milestones: 3.3.0, 3.4.0 May 8, 2024
@yangl yangl closed this Jun 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

doc-not-needed Your PR changes do not impact docs Stale

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants