[
https://issues.apache.org/jira/browse/YARN-9615?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17290963#comment-17290963
]
Peter Bacsko commented on YARN-9615:
------------------------------------
Thanks for the patch [~zhuqi].
Some comments:
1.
{noformat}
eventTypeMetricsMap.get(event.getType().getClass())
.incr(event.getType(),
(System.nanoTime() - startTime) / 1000);
{noformat}
I'm not 100% confident in this, but most of the time, we rely on {{Clock}}
implementations, like {{MonotonicClock}}. I suggest using
{{MonotonicClock.getTime()}}.
It might be a good idea to introduce a new method to {{AsyncDispatcher}} like
{{setClock()}} (mark it with VisibleForTesting). This way, you can replace the
Clock instance with a mock or something else, so testability is much easier.
2. Same thing applies to {{EventDispatcher}}.
3. Nit: {{public class DisableEventTypeMetrics implements EventTypeMetrics{}}
-- add space after "EventTypeMetrics"
4.
{noformat}
@Override
public void get(Enum type) {
}
{noformat}
If this method does nothing, pls. add a comment like "//nop" to the method body
(make it clear that no-op is normal).
5.
{noformat}
@Override
public void get(T type) {
}
@Override
public void getMetrics(MetricsCollector collector, boolean all) {
}
{noformat}
Same here, add a short "// nop" comment in the method bodies.
6. ResourceManager.java: {{import org.apache.hadoop.yarn.event.*;}} --> avoid
star imports
7. EventTypeMetrics.java:
{noformat}
void incr(T type, long processingTimeUs);
{noformat}
Nit: it's a minor thing, but if we can do it, let's write complete words, so
I'd opt for {{increment()}} instead of just {{incr()}}.
8. Very important: there are NO tests for either {{EventDispatcher}} or
{{AsyncDispatcher}}. Please add 1-2 unit tests that validate the correct
behavior (and think of #1 and you can use a mock {{Clock}} instance for
verification).
Also fix checkstyle and FindBugs issues.
> Add dispatcher metrics to RM
> ----------------------------
>
> Key: YARN-9615
> URL: https://issues.apache.org/jira/browse/YARN-9615
> Project: Hadoop YARN
> Issue Type: Task
> Reporter: Jonathan Hung
> Assignee: Qi Zhu
> Priority: Major
> Attachments: YARN-9615.001.patch, YARN-9615.002.patch,
> YARN-9615.003.patch, YARN-9615.poc.patch, screenshot-1.png
>
>
> It'd be good to have counts/processing times for each event type in RM async
> dispatcher and scheduler async dispatcher.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
