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

Refactored PulsarAdmin to use builder for configuration #1327

Merged
merged 2 commits into from
Apr 2, 2018
Merged

Refactored PulsarAdmin to use builder for configuration #1327

merged 2 commits into from
Apr 2, 2018

Conversation

merlimat
Copy link
Contributor

@merlimat merlimat commented Mar 4, 2018

Motivation

PulsarAdmin was taking a ClientConfiguration object from pulsar-client. The ClientConfiguration class has been now deprecated and in any case it was not really appropriate for the PulsarAdmin since it just needs to get the configuration for the authentication plugin

Modifications

  • Deprecated current constructors for PulsarAdmin
  • Added builder to configure and construct PulsarAdmin instance, consistent with PulsarClient API.
  • Updated all usages to new API

@merlimat merlimat added the type/cleanup Code or doc cleanups e.g. remove the outdated documentation or remove the code no longer in use label Mar 4, 2018
@merlimat merlimat added this to the 2.0.0-incubating milestone Mar 4, 2018
@merlimat merlimat self-assigned this Mar 4, 2018
@sijie
Copy link
Member

sijie commented Mar 4, 2018

This change might need to rebase to master to incorporate pulsar-functions.

Also master is broken due to two independent merges at #1314 and #1320 , I am fixing that at #1324

@merlimat merlimat force-pushed the client-admin branch 4 times, most recently from 466be41 to 556749e Compare March 30, 2018 19:15
@merlimat
Copy link
Contributor Author

retest this please

sijie
sijie approved these changes Apr 2, 2018
View reviewed changes
Copy link
Member

@sijie sijie left a comment

Choose a reason for hiding this comment

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

looks great! +1

@merlimat merlimat merged commit df3f757 into apache:master Apr 2, 2018
@sijie sijie mentioned this pull request Apr 4, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sijie sijie sijie approved these changes

@maskit maskit Awaiting requested review from maskit

@rdhabalia rdhabalia Awaiting requested review from rdhabalia

@jai1 jai1 Awaiting requested review from jai1

@saandrews saandrews Awaiting requested review from saandrews

@nkurihar nkurihar Awaiting requested review from nkurihar

@yush1ga yush1ga Awaiting requested review from yush1ga

Assignees

@merlimat merlimat

Labels
type/cleanup Code or doc cleanups e.g. remove the outdated documentation or remove the code no longer in use
Projects
None yet
Milestone
2.0.0-incubating
Development

Successfully merging this pull request may close these issues.
2 participants
@merlimat @sijie