[FLINK-7718] [flip6] Add JobVertexMetricsHandler to DispatcherRestEndpoint #5055
Conversation
…point Migrate logic in org.apache.flink.runtime.rest.handler.legacy.metrics.JobVertexMetricsHandler to new handler and add new handler to DispatcherRestEndpoint. Add common classes for remaining implementations of org.apache.flink.runtime.rest.handler.legacy.metrics.AbstractMetricsHandler, which require migration as well.
|
|
||
| final List<Metric> metrics = new ArrayList<>(requestedMetrics.size()); | ||
| for (final String requestedMetric : requestedMetrics) { | ||
| final String value = componentMetricStore.getMetric(requestedMetric, null); |
There was a problem hiding this comment.
If requestedMetric is an empty string, we will return HTTP code 404. This is different compared to the legacy handler. If this is a problem, I can change it to the old behavior (return empty string with HTTP 200).
There was a problem hiding this comment.
I think we actually have a slightly inconsistent behaviour here. In case that componentMetricStore or componentMetricStore.metrics is null we return an empty metrics object. But in case that a requested metric is not there (might not be registered or transferred yet) we fail with a 404. With the current way we handle a non-existent metric store, it would be more consistent to simply not report metrics which are not there.
Furthermore, it could also be the case that only a subset of the requested metrics is not yet registered.
There was a problem hiding this comment.
Changed the behavior:
- Unknown metrics are not included.
- If all metrics are unknown, an empty list is returned.
| /** | ||
| * Tests for {@link MetricsFilterParameter}. | ||
| */ | ||
| public class MetricsFilterParameterTest { |
There was a problem hiding this comment.
Should inherit from TestLogger.
…MetricsHeaders Strip the term Handler from the Header class. Also rename its subclasses.
There was a problem hiding this comment.
Thanks for your contribution @GJL. Changes look good. I only had a single comment concerning how we handle non existent metrics and non existent component metric store. Once this is resolved, we can merge the PR.
|
|
||
| final List<Metric> metrics = new ArrayList<>(requestedMetrics.size()); | ||
| for (final String requestedMetric : requestedMetrics) { | ||
| final String value = componentMetricStore.getMetric(requestedMetric, null); |
There was a problem hiding this comment.
I think we actually have a slightly inconsistent behaviour here. In case that componentMetricStore or componentMetricStore.metrics is null we return an empty metrics object. But in case that a requested metric is not there (might not be registered or transferred yet) we fail with a 404. With the current way we handle a non-existent metric store, it would be more consistent to simply not report metrics which are not there.
Furthermore, it could also be the case that only a subset of the requested metrics is not yet registered.
…obVertexMetricsMessageParameters
Brief change log
org.apache.flink.runtime.rest.handler.legacy.metrics.JobVertexMetricsHandlerto new handler and add new handler to DispatcherRestEndpoint.org.apache.flink.runtime.rest.handler.legacy.metrics.AbstractMetricsHandler,which require migration as well
Verifying this change
This change added tests and can be verified as follows:
DispatcherRestEndpointcurlthat JobVertexMetrics can be queried in FLIP-6.Does this pull request potentially affect one of the following parts:
@Public(Evolving): (yes / no)Documentation
CC: @tillrohrmann