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

[Proxy] Remove unnecessary Pulsar Client usage from Pulsar Proxy #13836

Conversation

lhotari
Copy link
Member

@lhotari lhotari commented Jan 19, 2022

Motivation

Pulsar Proxy will create a new Pulsar Client instance for every proxied connection.

Currently 2 threads will be created for every Pulsar Client instance for the internal/IO and external/listener executors. This causes a lot of overhead and inefficiency in the Pulsar Proxy.

It turns out that the PulsarClientImpl instance is unnecessary in Pulsar Proxy. The Proxy code doesn't need the instance at all and therefore it can be removed.

While working on the changes, it was noticed that the ConnectionPool cleanup code is wrong. It is closing the EventLoopGroup provided to the ConnectionPool although ConnectionPool doesn't create this resource. In addition, closing didn't close existing connections.

Additional context

This PR replaces PR #13834

Modifications

  • remove PulsarClientImpl usage from Pulsar Proxy completely
  • remove the shared timer since it's no more needed
  • fix ConnectionPool.close
  • fix checkstyle errors in ConnectionPool
  • fix threadsafety and state issues in ProxyConnection.close

@github-actions
Copy link

@lhotari:Thanks for your contribution. For this PR, do we need to update docs?
(The PR template contains info about doc, which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)

@lhotari lhotari added the doc-not-needed Your PR changes do not impact docs label Jan 19, 2022
Copy link
Contributor

@nicoloboschi nicoloboschi left a comment

Choose a reason for hiding this comment

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

LGTM

@github-actions
Copy link

@lhotari:Thanks for providing doc info!

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.

Great move!

Copy link
Contributor

@merlimat merlimat 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

@michaeljmarshall michaeljmarshall left a comment

Choose a reason for hiding this comment

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

LGTM

@lhotari lhotari merged commit 324aa1b into apache:master Jan 19, 2022
lhotari added a commit to lhotari/pulsar that referenced this pull request Feb 9, 2022
lhotari added a commit that referenced this pull request Feb 9, 2022
)

(cherry picked from commit 324aa1b)
(cherry picked from commit 25e6b65)
@lhotari lhotari added cherry-picked/branch-2.7 Archived: 2.7 is end of life release/2.7.5 cherry-picked/branch-2.8 Archived: 2.8 is end of life labels Feb 9, 2022
lhotari added a commit that referenced this pull request Feb 9, 2022
@lhotari lhotari added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label Feb 9, 2022
lhotari added a commit to datastax/pulsar that referenced this pull request Feb 9, 2022
…che#13836)

(cherry picked from commit 324aa1b)
(cherry picked from commit 25e6b65)
(cherry picked from commit 94fd1f5)
lhotari added a commit to datastax/pulsar that referenced this pull request Feb 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/client area/proxy cherry-picked/branch-2.7 Archived: 2.7 is end of life cherry-picked/branch-2.8 Archived: 2.8 is end of life cherry-picked/branch-2.9 Archived: 2.9 is end of life doc-not-needed Your PR changes do not impact docs release/2.7.5 release/2.8.3 release/2.9.2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants