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

[fix][cli] Pulsar shell: ensure admin client is recycled or disposed #17619

Merged

Conversation

nicoloboschi
Copy link
Contributor

Motivation

In Pulsar shell when creating a new admin client (using the admin API) is created for each command. There are two considerations:

  • the admin client is never closed (memory and tcp cnxs leak)
  • the admin client should be reused when using the same config

Modifications

  • Refactored the pulsar-admin main class (PulsarAdminTool) in order to being able to compare configurations and decide to keep or destroy the current admin client

The client is discarded when:

  1. You pass a inline parameter (e.g. --admin-url) that is different from the current one
  2. You change config (config use ...) without the comparison because a new PulsarAdminTool is created. This makes sense because when you change the config, you change the tenant or cluster.
  • Fixed another leak in for custom commands. They list of custom commands continues to grow for each command and it's never cleared
  • doc-not-needed

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Sep 13, 2022
Copy link
Contributor

@eolivelli eolivelli 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- this should go into 2.11 as well

@nicoloboschi nicoloboschi merged commit 1ff9fb6 into apache:master Sep 14, 2022
nicoloboschi added a commit that referenced this pull request Sep 14, 2022
…17619)

* [fix][cli] Pulsar shell: ensure admin client is recycled or disposed

(cherry picked from commit 1ff9fb6)
Boolean tlsAllowInsecureConnection;
String tlsTrustCertsFilePath;
Boolean tlsEnableHostnameVerification;
String tlsProvider;
Copy link
Member

Choose a reason for hiding this comment

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

Miss the TLS transport config here, let me make a PR to do that.

nicoloboschi added a commit to datastax/pulsar that referenced this pull request Sep 16, 2022
…pache#17619)

* [fix][cli] Pulsar shell: ensure admin client is recycled or disposed

(cherry picked from commit 1ff9fb6)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants