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

[improve][broker] PIP-307: Add proxy support for Java client #21789

Merged
merged 9 commits into from
Dec 23, 2023

Conversation

dragosvictor
Copy link
Contributor

@dragosvictor dragosvictor commented Dec 21, 2023

PIP-307

Motivation

PRs #21408 and #21682 implemented producer/consumer support as per the new load balancer transfer mechanism, described by PIP-307, but both assumed that the clients were directly connected to the brokers. This PR implements the respective functionality when connections are made via the Pulsar proxy.

Modifications

  • Proxying information is determined by the LookupService.getBroker implementations but is not forwarded to the outside callers. The PR changes this method's signature to return a new LookupTopicResult object, that contains all the pertinent information and allows for ease of extension in the future.
  • Cache the proxying information (a boolean flag only) in ConnectionHandler and use it when the connection is closed with a broker URL redirection.

Verifying this change

  • Make sure that the change passes the CI checks.

This change added tests and can be verified as follows:

  • Added proxy producer/consumer test, verifying that disconnecting the clients respects the assigned URL logical address as well as the proxy physical address. Verified test stability by running it 100 times in the CI runner.
  • Added a test to verify the clients fall back to direct broker connections when the proxy service drops. Note that the test does not mandate a service URL change in the client. Instead, it resolves the URL to the proxy the first time and to the broker the second time. Verified for stability by running it 100 times.

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: (dragosvictor#3)

Copy link

@dragosvictor 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 -->

@github-actions github-actions bot added doc-not-needed Your PR changes do not impact docs and removed doc-label-missing labels Dec 21, 2023
@dragosvictor dragosvictor changed the title PIP-307: Add proxy support for Java client [improve][broker] PIP-307: Add proxy support for Java client Dec 21, 2023
@dragosvictor dragosvictor marked this pull request as ready for review December 22, 2023 00:11
@merlimat merlimat added this to the 3.2.0 milestone Dec 22, 2023
@Technoboy- Technoboy- modified the milestones: 3.2.0, 3.3.0 Dec 22, 2023
@codecov-commenter
Copy link

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (8beac8b) 73.42% compared to head (a1e4810) 73.39%.
Report is 1 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #21789      +/-   ##
============================================
- Coverage     73.42%   73.39%   -0.04%     
- Complexity    32795    32797       +2     
============================================
  Files          1897     1898       +1     
  Lines        140656   140668      +12     
  Branches      15491    15492       +1     
============================================
- Hits         103282   103239      -43     
- Misses        29297    29353      +56     
+ Partials       8077     8076       -1     
Flag Coverage Δ
inttests 24.18% <53.12%> (-0.07%) ⬇️
systests 24.72% <53.12%> (-0.13%) ⬇️
unittests 72.69% <84.37%> (-0.01%) ⬇️

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

Files Coverage Δ
...tion/buffer/impl/TransactionBufferHandlerImpl.java 67.08% <100.00%> (ø)
...e/pulsar/client/impl/BinaryProtoLookupService.java 82.53% <100.00%> (ø)
...g/apache/pulsar/client/impl/HttpLookupService.java 81.25% <100.00%> (ø)
...g/apache/pulsar/client/impl/ConnectionHandler.java 86.77% <88.88%> (-0.52%) ⬇️
...g/apache/pulsar/client/impl/LookupTopicResult.java 80.00% <80.00%> (ø)
...rg/apache/pulsar/client/impl/PulsarClientImpl.java 74.30% <72.72%> (+0.19%) ⬆️

... and 61 files with indirect coverage changes

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 ready-to-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants