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

ZOOKEEPER-2822: Wrong ObjectName about MBeanServer in JMX module #294

Closed
wants to merge 3 commits into from
Closed

ZOOKEEPER-2822: Wrong ObjectName about MBeanServer in JMX module #294

wants to merge 3 commits into from

Conversation

asdf2014
Copy link
Member

The wrong ObjectName about MBeanServer in JMX module, should log4j:hierarchy=default rather than log4j:hiearchy=default.

@asdf2014 asdf2014 closed this Nov 18, 2018
@asdf2014 asdf2014 reopened this Nov 18, 2018
eolivelli
eolivelli requested changes Nov 18, 2018
View reviewed changes
Copy link
Contributor

@eolivelli eolivelli left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thank you @asdf2014

I left a comment.

Please also @hanm @anmolnar take a look

zookeeper-server/src/main/java/org/apache/zookeeper/jmx/ManagedUtil.java Outdated
@@ -71,7 +71,7 @@ public static void registerLog4jMBeans() throws JMException {
// org.apache.log4j.jmx.HierarchyDynamicMBean hdm = new org.apache.log4j.jmx.HierarchyDynamicMBean();
Object hdm = Class.forName("org.apache.log4j.jmx.HierarchyDynamicMBean").getDeclaredConstructor().newInstance();

ObjectName mbo = new ObjectName("log4j:hiearchy=default");
ObjectName mbo = new ObjectName("log4j:hierarchy=default");
Copy link
Contributor

Choose a reason for hiding this comment

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

Is change may have an impact on integrated systems.
Maybe we could make this name configurable so that people will be able to revert the name.
This change can be only in master (3.6.0) as it is a public configuration/API change

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi, @eolivelli . Thanks for your reviewing. Indeed, turning it into a configurable one would be very helpful in upgrading an integrated system. So, let's be clear, should we add a new JVM option or add a new environment variable to the operating system?

@eolivelli
Copy link
Contributor

I would go for a system property

@asdf2014
Copy link
Member Author

@eolivelli Yep, I agree with you.

eolivelli
eolivelli requested changes Nov 20, 2018
View reviewed changes
zookeeper-server/src/main/java/org/apache/zookeeper/jmx/ManagedUtil.java Outdated

ObjectName mbo = new ObjectName("log4j:hiearchy=default");
String mbean = System.getenv("zookeeper.jmx.log4j.mbean");
Copy link
Contributor

Choose a reason for hiding this comment

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

You should use System.getProperty(name,defaultvalue)

Copy link
Member Author

Choose a reason for hiding this comment

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

I would go for a system property

FYI, System.getenv() is for Operating System environment variables, whereas System.getProperty() is for JVM arguments which are passed as -DpropName=value to Java application launcher.

So, maybe use getenv() would be more reasonable.

Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I know in ZK it is common to use System Properties and not environment variables.

I think that the launcher for the server process is setting as system property every configuration parameter.

Do you have any example of usages of getEnv in ZooKeeper ?

Copy link
Member Author

Choose a reason for hiding this comment

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

@eolivelli Okay, I will modify it.

@eolivelli
Copy link
Contributor

We should also add the property to the docs

@asdf2014
Copy link
Member Author

@eolivelli Thanks for your comment. I will describe this variable in zookeeperJMX.md.

anmolnar
anmolnar requested changes Nov 20, 2018
View reviewed changes
Copy link
Contributor

@anmolnar anmolnar left a comment

Choose a reason for hiding this comment

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

Nice catch @asdf2014 ! Thanks for providing the patch.
I support the idea of making it configurable and for that please use System.getProperty(), it's standard across ZooKeeper.

anmolnar
anmolnar approved these changes Nov 21, 2018
View reviewed changes
Copy link
Contributor

@anmolnar anmolnar left a comment

Choose a reason for hiding this comment

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

+1

eolivelli
eolivelli approved these changes Nov 21, 2018
View reviewed changes
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

+1
awesome work @asdf2014

@asdf2014
Copy link
Member Author

@eolivelli @anmolnar Thanks for your approval.

@anmolnar
Copy link
Contributor

retest this please

1 similar comment
@anmolnar
Copy link
Contributor

retest this please

@asdf2014
Copy link
Member Author

Hi, @anmolnar @eolivelli . Is there anything I need to do before we merge this PR?

@eolivelli
Copy link
Contributor

@anmolnar green lights from CI

This is a trivial patch, do you need another committer ?

@asfgit asfgit closed this in 91c6cb2 Nov 27, 2018
asfgit pushed a commit that referenced this pull request Nov 27, 2018
@asdf2014 @anmolnar
ZOOKEEPER-2822: Wrong ObjectName about MBeanServer in JMX module
019e841 
The wrong `ObjectName` about `MBeanServer` in JMX module, should `log4j:hierarchy=default` rather than `log4j:hiearchy=default`.

Author: asdf2014 <benedictjin2016@gmail.com>

Reviewers: andor@apache.org

Closes #294 from asdf2014/ZOOKEEPER-2822 and squashes the following commits:

294cf53 [asdf2014] Using `System.getProperty` instead of `System.getenv` & doc this change
ed756a9 [asdf2014] Add `zookeeper.jmx.log4j.mbean`
82da83a [asdf2014] ZOOKEEPER-2822: Wrong `ObjectName` about `MBeanServer` in JMX module

(cherry picked from commit 91c6cb2)
Signed-off-by: Andor Molnar <andor@apache.org>
@anmolnar
Copy link
Contributor

Committed to master and 3.5 branches. Thanks @asdf2014 !

@asdf2014
Copy link
Member Author

You are welcome.

@asdf2014 asdf2014 deleted the ZOOKEEPER-2822 branch November 27, 2018 10:46
RokLenarcic pushed a commit to RokLenarcic/zookeeper that referenced this pull request Sep 3, 2022
@asdf2014 @RokLenarcic
ZOOKEEPER-2822: Wrong ObjectName about MBeanServer in JMX module
6c5fced 
The wrong `ObjectName` about `MBeanServer` in JMX module, should `log4j:hierarchy=default` rather than `log4j:hiearchy=default`.

Author: asdf2014 <benedictjin2016@gmail.com>

Reviewers: andor@apache.org

Closes apache#294 from asdf2014/ZOOKEEPER-2822 and squashes the following commits:

294cf53 [asdf2014] Using `System.getProperty` instead of `System.getenv` & doc this change
ed756a9 [asdf2014] Add `zookeeper.jmx.log4j.mbean`
82da83a [asdf2014] ZOOKEEPER-2822: Wrong `ObjectName` about `MBeanServer` in JMX module
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anmolnar anmolnar anmolnar approved these changes

@eolivelli eolivelli eolivelli approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@asdf2014 @eolivelli @anmolnar