Skip to content

[feat][cpp] Support multiple brokers in service URL#17162

Merged
BewareMyPower merged 3 commits intoapache:masterfrom
BewareMyPower:bewaremypower/cpp-multi-url
Aug 30, 2022
Merged

[feat][cpp] Support multiple brokers in service URL#17162
BewareMyPower merged 3 commits intoapache:masterfrom
BewareMyPower:bewaremypower/cpp-multi-url

Conversation

@BewareMyPower
Copy link
Contributor

@BewareMyPower BewareMyPower commented Aug 18, 2022

Motivation

It's a catchup for #3249.

Currently C++ client doesn't have strict validation on the service URL.
If multiple brokers are specified, only the 1st broker will be used.

Modifications

  • Add ServiceURI to support configuring multiple brokers in the
    service URL. See ServiceURITest for how to configure it.
  • Add ServiceNameResolver whose resolveHost method selects the
    next broker in a round robin way.
  • Since the broker's address for topic lookup is no longer the service
    URL itself, to handle the case when proxy is enabled, a getBroker
    method (like the same method in Java's LookupService), which returns
    the future of a pair of logical and physical addresses, is added to
    replace lookupAsync.
  • Apply ServiceNameResolver into ClientImpl and the
    LookupService implementations.
  • Rename BinaryLookupServiceTest to LookupServiceTest and add
    testMultiAddresses to test both BinaryProtoLookupService and
    HTTPLookupService that all available brokers will be accessed in a
    round robin way.

Behavior Change

This PR also changes the behavior when the user passed service URL.
Before this PR, creating a Client object will succeed but the methods to
create a producer or consumer will return a failed Result. After this PR,
an invalid_argument exception will be thrown.

TODO

This is the 1st part to support multiple brokers. Even with this patch,
if one of these hosts is not available, the topic lookup will fail
immediately instead of trying other broker. Since this patch already
includes many code changes, I will push another patch to fix it.

Documentation

Check the box below or label this PR directly.

Need to update docs?

  • doc-required
    (Your PR needs to update docs and you will update later)

  • doc-not-needed
    (Please explain why)

  • doc
    (Your PR contains doc changes)

  • doc-complete
    (Docs have been already added)

@BewareMyPower BewareMyPower self-assigned this Aug 18, 2022
@BewareMyPower BewareMyPower added this to the 2.12.0 milestone Aug 18, 2022
@BewareMyPower BewareMyPower added type/feature The PR added a new feature or issue requested a new feature component/client-c++ labels Aug 18, 2022
@BewareMyPower
Copy link
Contributor Author

The testWrongListener is affected, I will mark it as draft and fix it.

2022-08-18 13:23:38.989 INFO  [140182675973888] ConnectionPool:96 | Created connection for
/pulsar/pulsar-client-cpp/tests/ClientTest.cc:248: Failure
Expected equality of these values:
  ResultServiceUnitNotReady
    Which is: ServiceUnitNotReady
  client.createProducer(topic, producer)
    Which is: ConnectError

@BewareMyPower BewareMyPower marked this pull request as draft August 18, 2022 13:54
@BewareMyPower BewareMyPower marked this pull request as ready for review August 18, 2022 14:28
@BewareMyPower BewareMyPower force-pushed the bewaremypower/cpp-multi-url branch from b7cb675 to 43b166e Compare August 18, 2022 14:29
@BewareMyPower BewareMyPower marked this pull request as draft August 19, 2022 05:03
@BewareMyPower
Copy link
Contributor Author

Draft this PR until the Python tests are fixed.

### Motivation

It's a catchup for apache#3249.

Currently C++ client doesn't have strict validation on the service URL.
If multiple brokers are specified, only the 1st broker will be used.

### Modifications

- Add `ServiceURI` to support configuring multiple brokers in the
  service URL. See `ServiceURITest` for how to configure it.
- Add `ServiceNameResolver` whose `resolveHost` method selects the
  next broker in a round robin way.
- Since the broker's address for topic lookup is no longer the service
  URL itself, to handle the case when proxy is enabled, a `getBroker`
  method (like the same method in Java's `LookupService`), which returns
  the future of a pair of logical and physical addresses, is added to
  replace `lookupAsync`.
- Apply `ServiceNameResolver` into `ClientImpl` and the
  `LookupService` implementations.
- Rename `BinaryLookupServiceTest` to `LookupServiceTest` and add
  `testMultiAddresses` to test both `BinaryProtoLookupService` and
  `HTTPLookupService` that all available brokers will be accessed in a
  round robin way.

### TODO

This is the 1st part to support multiple brokers. Even with this patch,
if one of these hosts is not available, the topic lookup will fail
immediately instead of trying other broker. Since this patch already
includes many code changes, I will push another patch to fix it.
@BewareMyPower BewareMyPower force-pushed the bewaremypower/cpp-multi-url branch from 62d9fc3 to f2ddba8 Compare August 19, 2022 09:04
@BewareMyPower BewareMyPower marked this pull request as ready for review August 19, 2022 09:51
@BewareMyPower
Copy link
Contributor Author

@RobertIndie @merlimat @rdhabalia @gaoran10 Could any of you take a second look?

@BewareMyPower BewareMyPower merged commit 786ab30 into apache:master Aug 30, 2022
@BewareMyPower BewareMyPower deleted the bewaremypower/cpp-multi-url branch September 15, 2022 01:23
@BewareMyPower BewareMyPower modified the milestones: 2.12.0, 2.11.0 Sep 15, 2022
BewareMyPower added a commit that referenced this pull request Sep 15, 2022
### Motivation

It's a catchup for #3249.

Currently C++ client doesn't have strict validation on the service URL.
If multiple brokers are specified, only the 1st broker will be used.

### Modifications

- Add `ServiceURI` to support configuring multiple brokers in the
  service URL. See `ServiceURITest` for how to configure it.
- Add `ServiceNameResolver` whose `resolveHost` method selects the
  next broker in a round robin way.
- Since the broker's address for topic lookup is no longer the service
  URL itself, to handle the case when proxy is enabled, a `getBroker`
  method (like the same method in Java's `LookupService`), which returns
  the future of a pair of logical and physical addresses, is added to
  replace `lookupAsync`.
- Apply `ServiceNameResolver` into `ClientImpl` and the
  `LookupService` implementations.
- Rename `BinaryLookupServiceTest` to `LookupServiceTest` and add
  `testMultiAddresses` to test both `BinaryProtoLookupService` and
  `HTTPLookupService` that all available brokers will be accessed in a
  round robin way.

### TODO

This is the 1st part to support multiple brokers. Even with this patch,
if one of these hosts is not available, the topic lookup will fail
immediately instead of trying other broker. Since this patch already
includes many code changes, I will push another patch to fix it.

(cherry picked from commit 786ab30)
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Sep 15, 2022
@batilak
Copy link

batilak commented Aug 2, 2023

Hi @BewareMyPower : this change supports multiple Broker URLs for brokers present in same cluster or it even supports brokers from different clusters?

As this is not clearly mentioned in documentation. thanks, ...

@BewareMyPower
Copy link
Contributor Author

See my comment #3249 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cherry-picked/branch-2.11 doc-not-needed Your PR changes do not impact docs release/2.11.0 type/feature The PR added a new feature or issue requested a new feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants