-
Notifications
You must be signed in to change notification settings - Fork 9.1k
HADOOP-17133. Implement HttpServer2 metrics #2145
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
Conversation
Manually tested with HttpFS. |
💔 -1 overall
This message was automatically generated. |
...hadoop-hdfs-httpfs/src/main/java/org/apache/hadoop/fs/http/server/HttpFSServerWebServer.java
Outdated
Show resolved
Hide resolved
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -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.
LGTM
Do you plan to add tests?
...op-common-project/hadoop-common/src/main/java/org/apache/hadoop/http/HttpServer2Metrics.java
Outdated
Show resolved
Hide resolved
BTW, on the topic of metrics and HttpFS, look at #2069 . That's adding the ability of client-side code to collect stats exclusive for the specific operation (input stream, output stream, listLocatedStatus) with the integration with the core IO classes so that things pass through all the way. Ultimate Goal: a hive/spark/impala job will be able to report an aggregate summary of the IO operations performed during a query, ideally even including throttling/retry events. Nowhere near that level of collection yet, but the public API should be ready for review. |
I'm planning to add a regression test.
Thank you for your information. I missed that. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -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.
The change looks good to me. Just two minor comments/questions.
// Jetty StatisticsHandler should be the first handler. | ||
// The handler returns 503 if there is no next handler and the response is | ||
// not committed. In Apache Hadoop, there are some servlets that do not | ||
// commit (i.e. close) the response. Therefore the handler fails. |
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.
If I understand correctly, this paragraph explains why we need to put the StatisticsHandler as the first handler, right? Is it possible that we can have a unit test to reproduce the scenario where the response is not committed ?
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.
I think it is possible. I'll create a unit test to reproduce the scenario.
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.
- I found why the handler should be inserted and updated the explanation: https://www.eclipse.org/lists/jetty-users/msg06273.html
- Created a test case for the scenario: 34bb0a6
Thank you @Jing9 for your review.
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/http/HttpServer2.java
Show resolved
Hide resolved
Some servlets does not close the stream and then StatisticsHandler fails. Fixed the servlets.
🎊 +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.
Thank you for updating the patch! Only a minor comment about the metrics javadoc. +1 after addressing the comment and fixing the checkstyle warning.
...op-common-project/hadoop-common/src/main/java/org/apache/hadoop/http/HttpServer2Metrics.java
Outdated
Show resolved
Hide resolved
🎊 +1 overall
This message was automatically generated. |
Thank you @Jing9! |
JIRA: https://issues.apache.org/jira/browse/HADOOP-17133