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

[feat][broker] PIP-368: Support lookup based on the lookup properties #23223

Merged
merged 3 commits into from
Aug 28, 2024

Conversation

RobertIndie
Copy link
Member

@RobertIndie RobertIndie commented Aug 26, 2024

PIP: #23075

Motivation

This is the implementation for the PIP: #23075
Currently, the lookup process uses only the topic name as its parameter. However, to enhance this process, it's
beneficial for clients to provide additional information. This could be done by introducing the lookupProperties field
in the client configuration. Clients can then share these properties with the broker during lookup.

On the broker side, the broker could also contain some properties that are used for the lookup. We can also support the
lookupProperties for the broker. The broker can use these properties to make a better decision on which broker to
return.

Here is the rack-aware lookup scenario for using the client properties for the lookup:
Assuming there are two brokers that broker-0 configures the lookup property "rack" with "A" and broker-1 configures the
lookup property "rack" with "B". By using the lookup properties, clients can supply rack information during the lookup,
enabling the broker to identify and connect them to the nearest broker within the same rack. If a client that configures
the "rack" property with "A" connects to a lookup broker, the customized load manager can determine broker-0 as the
owner broker since the broker and the client have the same rack property.

Modifications

  • Add new configuration lookupProperties to the client. While looking up the broker, the client will send the properties
    to the broker through CommandLookupTopic request.
  • Add properties field to the CommandLookupTopic.
  • Add lookupProperties to the LookupOptions. The Load Manager implementation can access
    the properties through LookupOptions to make a better decision on which broker to return.
  • Introduce a new broker configuration lookupPropertyPrefix. Any broker configuration properties that start with the lookupPropertyPrefix
    will be included into the BrokerLookupData and be persisted in the metadata store. The broker can use these properties
    during the lookup.

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: RobertIndie#15

Co-authored-by: Yunze Xu xyzinfernity@163.com

@RobertIndie RobertIndie self-assigned this Aug 26, 2024
Copy link

@RobertIndie Please add the following content to your PR description and select a checkbox:

- [ ] `doc` <!-- Your PR contains doc changes -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->

@RobertIndie RobertIndie marked this pull request as ready for review August 27, 2024 10:48
@BewareMyPower BewareMyPower added this to the 4.0.0 milestone Aug 27, 2024
@RobertIndie RobertIndie added doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. and removed doc-label-missing labels Aug 27, 2024
@github-actions github-actions bot added doc-label-missing and removed doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. labels Aug 27, 2024
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 93.33333% with 2 lines in your changes missing coverage. Please review.

Project coverage is 74.53%. Comparing base (bbc6224) to head (baa7e94).
Report is 547 commits behind head on master.

Files Patch % Lines
...org/apache/pulsar/broker/ServiceConfiguration.java 87.50% 0 Missing and 1 partial ⚠️
...g/apache/pulsar/broker/lookup/TopicLookupBase.java 50.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #23223      +/-   ##
============================================
+ Coverage     73.57%   74.53%   +0.95%     
- Complexity    32624    34238    +1614     
============================================
  Files          1877     1922      +45     
  Lines        139502   144785    +5283     
  Branches      15299    15835     +536     
============================================
+ Hits         102638   107913    +5275     
+ Misses        28908    28595     -313     
- Partials       7956     8277     +321     
Flag Coverage Δ
inttests 27.66% <63.33%> (+3.07%) ⬆️
systests 24.69% <43.33%> (+0.37%) ⬆️
unittests 73.90% <93.33%> (+1.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...ker/loadbalance/extensions/BrokerRegistryImpl.java 83.96% <100.00%> (-0.80%) ⬇️
.../loadbalance/extensions/data/BrokerLookupData.java 90.47% <ø> (-9.53%) ⬇️
.../apache/pulsar/broker/namespace/LookupOptions.java 88.88% <100.00%> (+1.38%) ⬆️
...va/org/apache/pulsar/broker/service/ServerCnx.java 72.37% <100.00%> (+0.23%) ⬆️
...e/pulsar/client/impl/BinaryProtoLookupService.java 81.60% <100.00%> (-0.94%) ⬇️
...g/apache/pulsar/client/impl/ClientBuilderImpl.java 86.98% <100.00%> (+0.99%) ⬆️
...lsar/client/impl/conf/ClientConfigurationData.java 96.89% <100.00%> (+0.20%) ⬆️
...va/org/apache/pulsar/common/protocol/Commands.java 91.14% <100.00%> (+0.54%) ⬆️
...org/apache/pulsar/broker/ServiceConfiguration.java 98.95% <87.50%> (-0.45%) ⬇️
...g/apache/pulsar/broker/lookup/TopicLookupBase.java 69.23% <50.00%> (+1.81%) ⬆️

... and 534 files with indirect coverage changes

@RobertIndie RobertIndie merged commit 9a97c84 into apache:master Aug 28, 2024
56 of 58 checks passed
grssam pushed a commit to grssam/pulsar that referenced this pull request Sep 4, 2024
…apache#23223)

PIP: apache#23075

### Motivation

This is the implementation for the PIP: apache#23075
Currently, the lookup process uses only the topic name as its parameter. However, to enhance this process, it's
beneficial for clients to provide additional information. This could be done by introducing the `lookupProperties` field
in the client configuration. Clients can then share these properties with the broker during lookup.

On the broker side, the broker could also contain some properties that are used for the lookup. We can also support the
lookupProperties for the broker. The broker can use these properties to make a better decision on which broker to
return.

Here is the rack-aware lookup scenario for using the client properties for the lookup:
Assuming there are two brokers that broker-0 configures the lookup property "rack" with "A" and broker-1 configures the
lookup property "rack" with "B". By using the lookup properties, clients can supply rack information during the lookup,
enabling the broker to identify and connect them to the nearest broker within the same rack. If a client that configures
the "rack" property with "A" connects to a lookup broker, the customized load manager can determine broker-0 as the
owner broker since the broker and the client have the same rack property.

### Modifications

- Add new configuration `lookupProperties` to the client. While looking up the broker, the client will send the properties
to the broker through `CommandLookupTopic` request.
- Add `properties` field to the `CommandLookupTopic`.
- Add `lookupProperties` to the `LookupOptions`. The Load Manager implementation can access
the `properties` through `LookupOptions` to make a better decision on which broker to return.
- Introduce a new broker configuration `lookupPropertyPrefix`. Any broker configuration properties that start with the `lookupPropertyPrefix`
will be included into the `BrokerLookupData` and be persisted in the metadata store. The broker can use these properties
during the lookup.

Co-authored-by: Yunze Xu <xyzinfernity@163.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants