-
Notifications
You must be signed in to change notification settings - Fork 8.8k
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
HADOOP-16398. Exports Hadoop metrics to Prometheus #1170
Conversation
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, LGTM
Setting the Ozone label to get an Ozone test run too. But we are good with committing and I will go ahead and commit this soon. |
// TODO: Replace "hadoop.prometheus.endpoint.enabled" with | ||
// CommonConfigurationKeysPublic.HADOOP_PROMETHEUS_ENABLED when possible. | ||
conf.setBoolean("hadoop.prometheus.endpoint.enabled", false); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aajisaka Just trying to understand all the options here. I see we have 3 options of running this code.
- Ozone and HDFS as independent processes - No issues.
- HDFS starts first, and Ozone is enabled in the running process -- Works because the Prometheus servelet is loaded and running already.
- Ozone is running first -- and then HDFS starts up -- since they are independent processes it will work. That is, the port/prom , the port will be different so we are ok,
- There is no case, where both (Ozone and HDFS) are in the same process but Ozone starts first and then HDFS starts. Hence this setting is safe.
@elek , @adoroszlai Thoughts? I am going to commit this tomorrow since we are all in different time zones. Just making sure that my reasoning is correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I think your understanding is correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 (non-binding) from me
Thanks @anuengineer and @adamantal for the reviews. Merged. |
…hen host affinity is disabled (apache#1170) * Adding expiry check for unresponsive Cluster Manager when host affinity is off * Fixing after rebase * Addressing Ray's feedback * Updating javadocs * Nitpick improvements
JIRA: https://issues.apache.org/jira/browse/HADOOP-16398