Skip to content

MINOR: Fix JMX serialization by reverting #5011#5114

Merged
ijuma merged 1 commit into
apache:trunkfrom
rayokota:fix-jmx-serialization
Jun 2, 2018
Merged

MINOR: Fix JMX serialization by reverting #5011#5114
ijuma merged 1 commit into
apache:trunkfrom
rayokota:fix-jmx-serialization

Conversation

@rayokota

@rayokota rayokota commented Jun 1, 2018

Copy link
Copy Markdown
Contributor

A recent change returns an anonymous inner class which retains
a reference to the outer class, KafkaMbean, which is not serializable.

This commit reverts #5011 as a proper fix needs to probably do serialization lazily as well by overriding readObject and writeObject.

Committer Checklist (excluded from commit message)

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

@rayokota rayokota changed the title Fix JMX serialization MINOR: Fix JMX serialization Jun 1, 2018

@hachikuji hachikuji left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks, LGTM.

@ijuma

ijuma commented Jun 1, 2018

Copy link
Copy Markdown
Member

@apurvam, this looks related to the issue you were seeing in KSQL recently.

@apurvam

apurvam commented Jun 1, 2018

Copy link
Copy Markdown
Contributor

Nice! Thanks @rayokota and @ijuma !

@hachikuji

Copy link
Copy Markdown
Contributor

@rayokota Note the findbugs error.

@lindong28

Copy link
Copy Markdown
Member

Sorry for problem caused by that patch. Thanks much for catching this!

…y calculating JMX attributes"

This reverts commit c9ec292.
@rayokota rayokota force-pushed the fix-jmx-serialization branch from 911732c to 7a2f923 Compare June 1, 2018 23:22
@rayokota

rayokota commented Jun 1, 2018

Copy link
Copy Markdown
Contributor Author

@lindong28 , @radai-rosenblatt , I've reverted #5011 for now as the simple fix that I had was probably not going to give you the performance benefit that you want. Originally I thought that I could simply move the anonymous inner class to be a static class, but the class still needs to be serializable so right now referring to the Map<String, KafkaMetric> wouldn't work as KafkaMetric is not serializable.

If you still want to try to achieve lazy calculation of the MBeanAttributeInfo, then you'll probably want to override writeObject and readObject to do the serialization lazily as well. Since this is a more complicated fix, I've reverted for now and you can attempt this and verify that it gives you the performance gains you are looking for. If you could also verify that serialization does not break, that would be appreciated.

@rayokota rayokota changed the title MINOR: Fix JMX serialization MINOR: Fix JMX serialization by reverting #5011 Jun 1, 2018
@ijuma

ijuma commented Jun 2, 2018

Copy link
Copy Markdown
Member

JMX metrics read via RMI rely on Serialization. Simply using JConsole should trigger this path.

@lindong28

Copy link
Copy Markdown
Member

@rayokota Sure. Thanks for catching and fixing this issue! Radai will update the patch and add the serialization logic and the test.

@rayokota

rayokota commented Jun 2, 2018

Copy link
Copy Markdown
Contributor Author

retest this please

@ijuma ijuma merged commit adec6d6 into apache:trunk Jun 2, 2018
@ijuma

ijuma commented Jun 2, 2018

Copy link
Copy Markdown
Member

I merged this to trunk. Next step is for @radai-rosenblatt to resubmit as @lindong28 said.

@rayokota rayokota deleted the fix-jmx-serialization branch June 2, 2018 20:19
ying-zheng pushed a commit to ying-zheng/kafka that referenced this pull request Jul 6, 2018
…ributes (apache#5114)

This reverts commit c9ec292 (apache#5011). That
commit introduces an anonymous inner class which retains a
reference to the non-serializable outer class `KafkaMbean`
breaking Serialization. This means that reading JMX metrics
via JConsole or JmxTool no longer works since RMI relies
on Java Serialization.

Reviewers: Jason Gustafson <jason@confluent.io>, Dong Lin <lindong28@gmail.com>, Ismael Juma <ismael@juma.me.uk>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants