refactor: Move RebindSafeMBeanServer to presto-common#27173
refactor: Move RebindSafeMBeanServer to presto-common#27173nishithakbhaskaran merged 1 commit intoprestodb:masterfrom
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideRefactors the RebindSafeMBeanServer utility into presto-common and updates all connectors to use the shared implementation while cleaning up now-unnecessary module-level dependencies and duplicated classes. Class diagram for shared RebindSafeMBeanServer and connector factoriesclassDiagram
class MBeanServer
class RebindSafeMBeanServer {
}
class HiveConnectorFactory {
}
class CassandraConnectorFactory {
}
class JmxConnectorFactory {
}
class ThriftConnectorFactory {
}
class PinotConnectorFactory {
}
RebindSafeMBeanServer --> MBeanServer : wraps
HiveConnectorFactory --> RebindSafeMBeanServer : uses
CassandraConnectorFactory --> RebindSafeMBeanServer : uses
JmxConnectorFactory --> RebindSafeMBeanServer : uses
ThriftConnectorFactory --> RebindSafeMBeanServer : uses
PinotConnectorFactory --> RebindSafeMBeanServer : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Since
presto-commonnow depends oncom.facebook.airlift:log(even withprovidedscope), consider whether the logging inRebindSafeMBeanServercan be removed or injected from callers to keeppresto-commonfree of connector-level logging dependencies. - Double-check that all previous package paths for
RebindSafeMBeanServerusages (e.g.,...connector.jmx.util,...connector.thrift.util, etc.) have been updated consistently to the newpresto-commonlocation so there are no lingering references or duplicated helpers.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Since `presto-common` now depends on `com.facebook.airlift:log` (even with `provided` scope), consider whether the logging in `RebindSafeMBeanServer` can be removed or injected from callers to keep `presto-common` free of connector-level logging dependencies.
- Double-check that all previous package paths for `RebindSafeMBeanServer` usages (e.g., `...connector.jmx.util`, `...connector.thrift.util`, etc.) have been updated consistently to the new `presto-common` location so there are no lingering references or duplicated helpers.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Saved that user @KNagaVivek is from IBM |
996de1c to
100df7f
Compare
agrawalreetika
left a comment
There was a problem hiding this comment.
Clean up lgtm.
I have one suggestion - I think moving @SuppressWarnings("deprecation") from class-level to method-level for the three deprecated deserialize() methods would be better. Since this provides more targeted suppression.
100df7f to
efe87da
Compare
|
Thanks @agrawalreetika Moved @SuppressWarnings("deprecation") from class-level to method-level. |
Move RebindSafeMBeanServer from individual connector modules to presto-common/util to eliminate code duplication across 5 connectors.
efe87da to
0359564
Compare
hantangwangd
left a comment
There was a problem hiding this comment.
Thanks @KNagaVivek, lgtm!
|
Thanks @KNagaVivek . |
Description
Move
RebindSafeMBeanServerfrom individual connector modules topresto-common(util package) to eliminate code duplication.Motivation and Context
During code review, it was identified that
RebindSafeMBeanServerwas duplicated across 5 different connector modules (hive, cassandra, jmx, thrift-connector, and pinot-toolkit). This utility class wrapsMBeanServerto handle MBean re-registration scenarios gracefully.Impact
No functional changes. This is an internal refactoring only
Test Plan
Verified that all affected connectors build successfully.
Contributor checklist
Release Notes
Summary by Sourcery
Consolidate the RebindSafeMBeanServer utility into presto-common and update connectors to use the shared implementation, eliminating duplicated code.
Enhancements:
Build: