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

KAFKA-18077: Remove deprecated JmxReporter(String) #17923

Open
wants to merge 4 commits into
base: trunk
Choose a base branch
from

Conversation

clarkwtc
Copy link
Contributor

Remove the following code about the deprecated JmxReporter(String).

  • Remove initial JmxReporter constructor with string and refactor default constructor.
  • Remove related test fucntion for testDeprecatedJmxPrefixWithDefaultMetrics.

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

Copy link
Contributor

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

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

@clarkwtc please update upgrade.html also

@clarkwtc
Copy link
Contributor Author

@chia7712 I have updated the upgrade.html, please review it.

Copy link
Contributor

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

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

@clarkwtc thanks for your patch. please rebase code

public JmxReporter(String prefix) {
this.prefix = prefix != null ? prefix : "";
}
public JmxReporter() {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please initialize "prefix" to empty string and then remove this default constructor as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chia7712 I got it; I missed initializing "prefix" to empty string, thanks.

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.

2 participants