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] Skip loading the NAR packages if not configured #21867

Merged

Conversation

BewareMyPower
Copy link
Contributor

Motivation

When loading NAR packages, including broker interceptors, additional servlets and protocol handlers, even if they are not configured, the NAR packages are still loaded, which is unnecessary and slows the restarting of a broker.

Modifications

Skip searching the directories and loading NAR packages when no plugin is configured, including:

  • BrokerInterceptors#load
  • AdditionalServlets#load
  • ProtocolHandlers#load

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository:

### Motivation

When loading NAR packages, including broker interceptors, additional
servlets and protocol handlers, even if they are not configured, the NAR
packages are still loaded, which is unnecessary and slows the restarting
of a broker.

### Modifications

Skip searching the directories and loading NAR packages when no plugin
is configured, including:
- `BrokerInterceptors#load`
- `AdditionalServlets#load`
- `ProtocolHandlers#load`
@BewareMyPower BewareMyPower self-assigned this Jan 9, 2024
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jan 9, 2024
@BewareMyPower BewareMyPower added type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages area/broker release/3.0.3 release/3.1.3 labels Jan 9, 2024
@BewareMyPower BewareMyPower added this to the 3.3.0 milestone Jan 9, 2024
@lhotari
Copy link
Member

lhotari commented Jan 9, 2024

@BewareMyPower Nice improvement. How about applying this also to src/main/java/org/apache/pulsar/proxy/extensions/ProxyExtensions.java. ?
Here:

public static ProxyExtensions load(ProxyConfiguration conf) throws IOException {
ExtensionsDefinitions definitions =
ProxyExtensionsUtils.searchForExtensions(
conf.getProxyExtensionsDirectory(), conf.getNarExtractionDirectory());
ImmutableMap.Builder<String, ProxyExtensionWithClassLoader> extensionsBuilder = ImmutableMap.builder();
conf.getProxyExtensions().forEach(extensionName -> {
ProxyExtensionMetadata definition = definitions.extensions().get(extensionName);
if (null == definition) {
throw new RuntimeException("No extension is found for extension name `" + extensionName
+ "`. Available extensions are : " + definitions.extensions());
}
ProxyExtensionWithClassLoader extension;
try {
extension = ProxyExtensionsUtils.load(definition, conf.getNarExtractionDirectory());
} catch (IOException e) {
log.error("Failed to load the extension for extension `" + extensionName + "`", e);
throw new RuntimeException("Failed to load the extension for extension name `" + extensionName + "`");
}
if (!extension.accept(extensionName)) {
extension.close();
log.error("Malformed extension found for extensionName `" + extensionName + "`");
throw new RuntimeException("Malformed extension found for extension name `" + extensionName + "`");
}
extensionsBuilder.put(extensionName, extension);
log.info("Successfully loaded extension for extension name `{}`", extensionName);
});
return new ProxyExtensions(extensionsBuilder.build());
}

@BewareMyPower
Copy link
Contributor Author

@lhotari Good suggestion. I applied the check to ProxyExtensions now.

@codecov-commenter
Copy link

Codecov Report

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

Comparison is base (cea5c93) 73.59% compared to head (40f67bd) 73.61%.
Report is 5 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #21867      +/-   ##
============================================
+ Coverage     73.59%   73.61%   +0.01%     
- Complexity    32323    32347      +24     
============================================
  Files          1858     1859       +1     
  Lines        138174   138281     +107     
  Branches      15148    15156       +8     
============================================
+ Hits         101696   101801     +105     
+ Misses        28608    28598      -10     
- Partials       7870     7882      +12     
Flag Coverage Δ
inttests 24.10% <4.47%> (-0.15%) ⬇️
systests 23.71% <16.41%> (-0.08%) ⬇️
unittests 72.90% <62.68%> (+0.03%) ⬆️

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

Files Coverage Δ
...pache/pulsar/broker/protocol/ProtocolHandlers.java 83.33% <100.00%> (+0.57%) ⬆️
...ervice/AbstractDispatcherSingleActiveConsumer.java 90.55% <100.00%> (ø)
...va/org/apache/pulsar/broker/service/ServerCnx.java 72.33% <100.00%> (-0.07%) ⬇️
...rg/apache/pulsar/client/impl/TopicListWatcher.java 67.85% <100.00%> (+1.19%) ⬆️
...pache/pulsar/proxy/extensions/ProxyExtensions.java 82.14% <100.00%> (+0.66%) ⬆️
...rg/apache/pulsar/broker/service/AbstractTopic.java 87.86% <0.00%> (-0.15%) ⬇️
...he/pulsar/broker/intercept/BrokerInterceptors.java 61.53% <33.33%> (-6.29%) ⬇️
.../broker/web/plugin/servlet/AdditionalServlets.java 63.41% <25.00%> (-3.26%) ⬇️
...ar/client/impl/PatternMultiTopicsConsumerImpl.java 82.70% <61.22%> (-5.26%) ⬇️

... and 79 files with indirect coverage changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/broker cherry-picked/branch-3.0 cherry-picked/branch-3.1 cherry-picked/branch-3.2 doc-not-needed Your PR changes do not impact docs ready-to-test release/3.0.3 release/3.1.3 release/3.2.1 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.

7 participants