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.

Sign up for GitHub

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

Jump to bottom

[improve][proxy] Reuse authentication instance in pulsar-proxy #23113

Merged
merged 3 commits into from
Aug 14, 2024
Merged

[improve][proxy] Reuse authentication instance in pulsar-proxy #23113

merged 3 commits into from
Aug 14, 2024

Conversation

equanz
Copy link
Contributor

@equanz equanz commented Aug 1, 2024 •
edited
Loading

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.

pulsar/pulsar-client/src/main/java/org/apache/pulsar/client/impl/PulsarClientImpl.java

Lines 911 to 918 in 92bcd07

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
@equanz equanz mentioned this pull request Aug 1, 2024
Closed
@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
lhotari
lhotari approved these changes Aug 5, 2024
View reviewed changes
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?

massakam
massakam approved these changes Aug 5, 2024
View reviewed changes
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 •
edited
Loading

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

Technoboy-
Technoboy- approved these changes Aug 7, 2024
View reviewed 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.

👍

nodece
nodece approved these changes Aug 9, 2024
View reviewed changes
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
64 of 65 checks passed
@equanz equanz deleted the reuse_authentication_instance_in_proxy branch August 14, 2024 02:12
Technoboy- pushed a commit that referenced this pull request Aug 14, 2024
@equanz @Technoboy-
[improve][proxy] Reuse authentication instance in pulsar-proxy (#23113)
d5833f9
lhotari pushed a commit that referenced this pull request Aug 14, 2024
@equanz @lhotari
[improve][proxy] Reuse authentication instance in pulsar-proxy (#23113)
b805a4a 
(cherry picked from commit 3e461c0)
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Aug 16, 2024
@equanz @nikhil-ctds
[improve][proxy] Reuse authentication instance in pulsar-proxy (apach…
391edcf 
…e#23113)

(cherry picked from commit 3e461c0)
(cherry picked from commit b805a4a)
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Aug 16, 2024
@equanz @nikhil-ctds
[improve][proxy] Reuse authentication instance in pulsar-proxy (apach…
2ebbb7a 
…e#23113)

(cherry picked from commit 3e461c0)
(cherry picked from commit b805a4a)
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Aug 16, 2024
@equanz @nikhil-ctds
[improve][proxy] Reuse authentication instance in pulsar-proxy (apach…
345dd4e 
…e#23113)

(cherry picked from commit 3e461c0)
(cherry picked from commit b805a4a)
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Aug 20, 2024
@equanz @srinath-ctds
[improve][proxy] Reuse authentication instance in pulsar-proxy (apach…
b04b905 
…e#23113)

(cherry picked from commit 3e461c0)
(cherry picked from commit b805a4a)
grssam pushed a commit to grssam/pulsar that referenced this pull request Sep 4, 2024
@equanz @grssam
[improve][proxy] Reuse authentication instance in pulsar-proxy (apach…
bf108fb 
…e#23113)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lhotari lhotari lhotari approved these changes

@Technoboy- Technoboy- Technoboy- approved these changes

@nodece nodece nodece approved these changes

@massakam massakam massakam approved these changes

@michaeljmarshall michaeljmarshall Awaiting requested review from michaeljmarshall

Assignees

@equanz equanz

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
Milestone
4.0.0
Development

Successfully merging this pull request may close these issues.
6 participants
@equanz @codecov-commenter @lhotari @Technoboy- @nodece @massakam