You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@zookeeper.apache.org by GitBox <gi...@apache.org> on 2022/01/03 03:13:26 UTC
[GitHub] [zookeeper] gforceee opened a new pull request #1276: ZOOKEEPER-3497:Change MBeanRegistry to a standard singleton
gforceee opened a new pull request #1276:
URL: https://github.com/apache/zookeeper/pull/1276
As the [ZOOKEEPER-3497](https://issues.apache.org/jira/browse/ZOOKEEPER-3497) discuss, the MBeanRegistry is a singleton, so i change the constructor of MBeanRegistry to private method.
I would like to hear any feedback, please help review and let me know if additional changes are required.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] maoling commented on pull request #1276: ZOOKEEPER-3497:Change MBeanRegistry to a standard singleton
Posted by GitBox <gi...@apache.org>.
maoling commented on pull request #1276:
URL: https://github.com/apache/zookeeper/pull/1276#issuecomment-1001316154
If no other concerns, I'll merge it at this weekend(01-02)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] maoling closed pull request #1276: ZOOKEEPER-3497:Change MBeanRegistry to a standard singleton
Posted by GitBox <gi...@apache.org>.
maoling closed pull request #1276:
URL: https://github.com/apache/zookeeper/pull/1276
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] maoling closed pull request #1276: ZOOKEEPER-3497:Change MBeanRegistry to a standard singleton
Posted by GitBox <gi...@apache.org>.
maoling closed pull request #1276:
URL: https://github.com/apache/zookeeper/pull/1276
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] tisonkun commented on pull request #1276: ZOOKEEPER-3497:Change MBeanRegistry to a standard singleton
Posted by GitBox <gi...@apache.org>.
tisonkun commented on pull request #1276:
URL: https://github.com/apache/zookeeper/pull/1276#issuecomment-931856785
cc @maoling @eolivelli could you please take a look at this PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] eolivelli commented on pull request #1276: ZOOKEEPER-3497:Change MBeanRegistry to a standard singleton
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #1276:
URL: https://github.com/apache/zookeeper/pull/1276#issuecomment-1003911622
> If no other concerns, I'll merge it at this weekend(01-02)
This is not good.
We have rules, and we cannot merge non trivial PRs without two binding reviews
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] maoling commented on a change in pull request #1276: ZOOKEEPER-3497:Change MBeanRegistry to a standard singleton
Posted by GitBox <gi...@apache.org>.
maoling commented on a change in pull request #1276:
URL: https://github.com/apache/zookeeper/pull/1276#discussion_r777801548
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/jmx/MBeanRegistry.java
##########
@@ -43,28 +43,26 @@
public static final String DOMAIN = "org.apache.ZooKeeperService";
private static final Logger LOG = LoggerFactory.getLogger(MBeanRegistry.class);
- private static volatile MBeanRegistry instance = new MBeanRegistry();
private final Object LOCK = new Object();
private Map<ZKMBeanInfo, String> mapBean2Path = new ConcurrentHashMap<ZKMBeanInfo, String>();
private MBeanServer mBeanServer;
- /**
- * Useful for unit tests. Change the MBeanRegistry instance
- *
- * @param instance new instance
- */
- public static void setInstance(MBeanRegistry instance) {
- MBeanRegistry.instance = instance;
+ private static class MBeanRegistryHolder {
Review comment:
@eolivelli This is just a singleton style a programmer prefers. Look at this:
[1] https://en.wikipedia.org/wiki/Initialization-on-demand_holder_idiom#Example_Java_Implementation
[2] https://stackoverflow.com/questions/17799976/why-is-static-inner-class-singleton-thread-safe
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] eolivelli commented on a change in pull request #1276: ZOOKEEPER-3497:Change MBeanRegistry to a standard singleton
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #1276:
URL: https://github.com/apache/zookeeper/pull/1276#discussion_r777322284
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/jmx/MBeanRegistry.java
##########
@@ -43,28 +43,26 @@
public static final String DOMAIN = "org.apache.ZooKeeperService";
private static final Logger LOG = LoggerFactory.getLogger(MBeanRegistry.class);
- private static volatile MBeanRegistry instance = new MBeanRegistry();
private final Object LOCK = new Object();
private Map<ZKMBeanInfo, String> mapBean2Path = new ConcurrentHashMap<ZKMBeanInfo, String>();
private MBeanServer mBeanServer;
- /**
- * Useful for unit tests. Change the MBeanRegistry instance
- *
- * @param instance new instance
- */
- public static void setInstance(MBeanRegistry instance) {
- MBeanRegistry.instance = instance;
+ private static class MBeanRegistryHolder {
Review comment:
Why do we need this additional class?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org