[
https://issues.apache.org/jira/browse/KNOX-940?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16035670#comment-16035670
]
Mohammad Kamrul Islam commented on KNOX-940:
--------------------------------------------
Thanks [~moresandeep] for detailed review comments. I accepted most of your
comment and plan to include in the next revision. Also I added few comments.
Can you please review those? After getting your feedback, I will upload the
patch accordingly.
* pom issue: as you found, I was testing against 0.12. It should be 0.13.0
instead. Updating the patch accordingly.
>>> Can you also briefly describe the overall architecture and the url pattern
>>> that can be used to access the metrics, it will be helpful for the review
>>> and for future reference.
Once the patch is cleared, I will write a doc explaining the details. I used
the following two commands for testing, planning to add more such health end
points:
* curl -iv
'https://hadooppoc03-sjc1.prod.uber.internal:8443/gateway/health/v0/metrics?pretty=true'
* curl –iv
'https://hadooppoc03-sjc1.prod.uber.internal:8443/gateway/health/v0/ping'
>>> In class DefaultMetricsService, any specific reason to make 'MetricRegistry
>>> metrics' static.
Listener classes needs to access this statically. There could be better way but
I followed the pattern defined in dropwizard tutorial
(http://metrics.dropwizard.io/3.1.0/manual/servlets/#adminservlet)
>>> HealthServiceMessages class, the messages are too generic like "Exception
>>> occurred" it would be good to have messages which indicate the cause of
>>> failure.Method "basicInfo(String original)" boes not appear to be used.
Will be addressed in the new patch.
>>>MetricsResource class:
>>> Path has hard coded version (/v0/metrics), if the version is hard coded do
>>> we really need it?
I don’t have any strong opinion here. I came from a perspective that if we want
to support backward compatibility in future for REST end-points, we can use
version in the API. For each version, we will have a separate Resource file.
Each resource could be associated with one version.
However, if you think it is not needed for Knox, I can take that out. Or you
can suggest alternative approach that I can follow.
>>> Using instanceof is probably a bad idea as pointed by Larry McCay recently,
>>> it breaks the Open Close Principle and Liskov's Substitution Principle
Good point. I believe most of the doc was primarily talking about class
hierarchy and how instanceOf could be a bad choice. In this instance, I need to
cast and I want to verify if cast exception would occur. If you still think, it
will be an issue, I’m open for any alternative option.
>>> Why do we have a doPost method when it does not handle POST (or may be I
>>> read it wrong)
>>>doGet() method produces JSON as well as XML, as per the method annotations
>>>but the response content type is only JSON
I borrowed both patterns that was used for similar Knox services (i.e.
knoxtoken service code – TokenResource.java). I don’t need those explicitly but
followed the same styles.
>>>in the method getMetrics(), on error the response is
>>>Response.ok().entity(String.format("Failed to reply correctly due to : %s ",
>>>ioe)).build(), shouldn't it be something like a Response.serverError()..
will be addressed in new patch.
>>>PingResource class
>>>Can you explain the purpose of this class
The intention is to provide hearbeat type of query quickly. Again following the
pattern mentioned in dropwizard tutorial
(http://metrics.dropwizard.io/3.1.0/manual/servlets/#pingservlet).
>>>Path has hard-coded version (/v0/ping), if the version is hard coded do we
>>>really need it ?
>>> Why do we have a doPost method when it does not handle POST (or may be I
>>> read it wrong)
>>> in the method getPingResponse(), on error the response is
>>> Response.ok().entity(String.format("Failed to reply correctly due to : %s
>>> ", ioe)).build(), shouldn't it be something like a Response.serverError().
Replied before.
> Support REST access exposing metrics
> ------------------------------------
>
> Key: KNOX-940
> URL: https://issues.apache.org/jira/browse/KNOX-940
> Project: Apache Knox
> Issue Type: Task
> Affects Versions: 0.11.0
> Reporter: Mohammad Kamrul Islam
> Assignee: Mohammad Kamrul Islam
> Fix For: 0.13.0
>
> Attachments: KNOX-940.patch
>
>
> KNOX-643 added the support to publish metrics. However, the metrics are not
> accessible through REST. This JIRA is to add REST support.
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
