-
Notifications
You must be signed in to change notification settings - Fork 9.2k
HDFS-15711. Add Metrics to HttpFS Server. #2521
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
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
34be9cf to
c2cc329
Compare
|
🎊 +1 overall
This message was automatically generated. |
jbrennan333
left a comment
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 @amahussein. See inline comments.
...ct/hadoop-hdfs-httpfs/src/main/java/org/apache/hadoop/fs/http/server/HttpFSServerWebApp.java
Outdated
Show resolved
Hide resolved
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.
It's not clear to me why we need this metricsGetter. Seems to add complexity with no added value. Can't you just call the appropriate HttpFSServerWebApp.get().getMetrics().get* function wherever this is used?
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 that the best way to check the metrics is to inject code in getStatus. This reduced the diff significantly. getStatus() accepts the cmd as a string, so adding if-then-else for each cmd will be more difficult to maintain. Finally, it is easier to add a command to the metricsGetter to extend the set of tests.
Also, the default callable saves NPE in case the command is not mapped to a metricGetter.
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.
Oh I see that makes sense for getStatus(). I was looking at testMkdirs() when I was trying to understand why you were using it, and it didn't make sense to me. I don't think you should use the metricsGetter in testMkdirs(), or at least not use getOrDefault() - the default case here does not make sense - it renders that part of the test meaningless. I'd prefer to just get the ops directly in this case.
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 @jbrennan333 .
This makes sense.
I fixed testMkdirs, createDirWithHttp, testGlobFilter, and testHdfsAccess
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.
Seems like extra blank lines were added by accident.
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.
done
...ct/hadoop-hdfs-httpfs/src/main/java/org/apache/hadoop/fs/http/server/HttpFSServerWebApp.java
Outdated
Show resolved
Hide resolved
...ct/hadoop-hdfs-httpfs/src/main/java/org/apache/hadoop/fs/http/server/HttpFSServerWebApp.java
Outdated
Show resolved
Hide resolved
c2cc329 to
b833540
Compare
|
🎊 +1 overall
This message was automatically generated. |
b833540 to
989b16e
Compare
|
🎊 +1 overall
This message was automatically generated. |
jbrennan333
left a comment
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 for the update @amahussein!
+1 this looks good to me.
NOTICE
Please create an issue in ASF JIRA before opening a pull request,
and you need to set the title of the pull request which starts with
the corresponding JIRA issue number. (e.g. HADOOP-XXXXX. Fix a typo in YYY.)
For more details, please see https://cwiki.apache.org/confluence/display/HADOOP/How+To+Contribute