Skip to content

Conversation

@equanz
Copy link
Contributor

@equanz equanz commented Aug 1, 2024

Motivation

In #12245 , try to remove redundant authentication instances and reuse it. However, in #15824 , the creation of authentication instances per DirectProxyHandler was reintroduced.
In #13836 , PulsarClient was removed from pulsar-proxy. As a result, this section (close of authentication instance) is no longer executed in pulsar-proxy. So, there is no need to create these instances per Client connections.

if (conf != null && conf.getAuthentication() != null) {
try {
conf.getAuthentication().close();
} catch (Throwable t) {
log.warn("Failed to close authentication", t);
throwable = t;
}
}

Modifications

  • Create an authentication instance at ProxyServiceStarter
  • Use this instance in ProxyService and WebServer

Verifying this change

  • Make sure that the change passes the CI checks.

This change added tests and can be verified as follows:

  • Add tests for org.apache.pulsar.proxy.server.ProxyServiceStarter#start

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: equanz#8

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Aug 1, 2024
@massakam massakam added type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages area/proxy ready-to-test labels Aug 5, 2024
@massakam massakam added this to the 3.4.0 milestone Aug 5, 2024
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. @michaeljmarshall do you have a chance to review?

Copy link
Contributor

@massakam massakam left a comment

Choose a reason for hiding this comment

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

LGTM

@codecov-commenter
Copy link

codecov-commenter commented Aug 5, 2024

Codecov Report

Attention: Patch coverage is 61.53846% with 10 lines in your changes missing coverage. Please review.

Project coverage is 73.45%. Comparing base (bbc6224) to head (04df95b).
Report is 678 commits behind head on master.

Files with missing lines Patch % Lines
...pache/pulsar/proxy/server/ProxyServiceStarter.java 50.00% 10 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #23113      +/-   ##
============================================
- Coverage     73.57%   73.45%   -0.12%     
- Complexity    32624    33236     +612     
============================================
  Files          1877     1919      +42     
  Lines        139502   144110    +4608     
  Branches      15299    15747     +448     
============================================
+ Hits         102638   105858    +3220     
- Misses        28908    30141    +1233     
- Partials       7956     8111     +155     
Flag Coverage Δ
inttests 27.47% <61.53%> (+2.89%) ⬆️
systests 24.76% <30.76%> (+0.44%) ⬆️
unittests 72.53% <15.38%> (-0.32%) ⬇️

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

Files with missing lines Coverage Δ
.../apache/pulsar/proxy/server/AdminProxyHandler.java 63.92% <100.00%> (-10.18%) ⬇️
...apache/pulsar/proxy/server/DirectProxyHandler.java 48.52% <100.00%> (-24.59%) ⬇️
...a/org/apache/pulsar/proxy/server/ProxyService.java 59.54% <100.00%> (-19.63%) ⬇️
...pache/pulsar/proxy/server/ProxyServiceStarter.java 46.41% <50.00%> (-24.28%) ⬇️

... and 513 files with indirect coverage changes

Copy link
Contributor

@Technoboy- Technoboy- left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Member

@nodece nodece left a comment

Choose a reason for hiding this comment

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

LGTM

@Technoboy- Technoboy- merged commit 3e461c0 into apache:master Aug 14, 2024
@equanz equanz deleted the reuse_authentication_instance_in_proxy branch August 14, 2024 02:12
lhotari pushed a commit that referenced this pull request Aug 14, 2024
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Aug 16, 2024
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Aug 16, 2024
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Aug 16, 2024
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Aug 20, 2024
grssam pushed a commit to grssam/pulsar that referenced this pull request Sep 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/proxy cherry-picked/branch-3.0 cherry-picked/branch-3.3 doc-not-needed Your PR changes do not impact docs ready-to-test release/3.0.7 release/3.3.2 type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants