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

Lazily init PulsarAdmin in PulsarAdminTool #9312

Merged
merged 6 commits into from
Jan 28, 2021

Conversation

eolivelli
Copy link
Contributor

@eolivelli eolivelli commented Jan 25, 2021

Motivation

pulsar-admin (PulsarAdminTool) initialises eagerly the PulsarAdmin object and some of the the underlying REST API intefaces.
This initialisation process triggers lot of resource loading (like SSL/RESTAPI classes....) that slows down the JVM even for stuff that is not needed.
Also there are shutdown hooks that are useless by they are executed while existing from the command.

Removing initialisation of useless stuff helps in having a better bootstrap time, especially in case that you are not performing API calls, like when you are learning the tool and you make lots of syntax errors.

Modifications

  • Initialise as lazily as possible PulsarAdmin
  • Make PulsarAdminTool#main "testable" by allowing it to not call System.exit
  • Use halt instead of exit in order to not trigger shutdown hooks

Verifying this change

This change is a trivial rework / code cleanup, but I have added tests for parts that have been touched and had not unit tests.

@sijie sijie added this to the 2.8.0 milestone Jan 25, 2021
@sijie sijie added area/admin type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages labels Jan 25, 2021
@wolfstudy
Copy link
Member

Thanks @eolivelli work for this, please fix the License header error:

Error:  COMPILATION ERROR : 
Error:  /home/runner/work/pulsar/pulsar/pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdTopics.java:[1317,13] admin has private access in org.apache.pulsar.admin.cli.CmdBase
Error:  /home/runner/work/pulsar/pulsar/pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdNamespaces.java:[1109,13] admin has private access in org.apache.pulsar.admin.cli.CmdBase
Error:  Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.8.1:compile (default-compile) on project pulsar-client-tools: Compilation failure: Compilation failure: 
Error:  /home/runner/work/pulsar/pulsar/pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdTopics.java:[1317,13] admin has private access in org.apache.pulsar.admin.cli.CmdBase
Error:  /home/runner/work/pulsar/pulsar/pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdNamespaces.java:[1109,13] admin has private access in org.apache.pulsar.admin.cli.CmdBase

@eolivelli
Copy link
Contributor Author

@wolfstudy thank you for the heads up, I have to rebase on master, probably some other changes went in.

@eolivelli
Copy link
Contributor Author

@wolfstudy I have update this patch, thanks PTAL

@eolivelli eolivelli force-pushed the fix/small-pulsar-admin-enhancements-2 branch from 9dc10f5 to 5c570ab Compare January 28, 2021 07:43
@eolivelli
Copy link
Contributor Author

This patch is good to go

@eolivelli
Copy link
Contributor Author

I would be interested in sending a backport for 2.7
would it be accepted ?
I guess 2.7.x will stay for quite a log time, having a faster pulsar-admin would be cool

@sijie sijie merged commit 232b324 into apache:master Jan 28, 2021
@sijie
Copy link
Member

sijie commented Jan 28, 2021

@eolivelli I would suggest leaving this change in master for now. We can backport it when it becomes a challenge when cherry-picking changes.

@eolivelli
Copy link
Contributor Author

Good point. No need to pick it to 2.7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/admin 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.

None yet

5 participants