Ngone51 commented on pull request #33116:
URL: https://github.com/apache/spark/pull/33116#issuecomment-880532381
> (a) we should adhere to the Timer interface, which already exposes the
unitless methods,
Although the Timer exposes the unitless methods, it doesn't mean the methods
don't have the time unit. Actually, I think this the key point that why we
misused the Timer in the first place. The timer has hidden the time unit. So,
wouldn't it be clearer if we force a unit requirement while creating the (our
custom) Timer?
> (b) the whole issue here is that these metrics themselves are trying to
define their unit, and if the caller supplies a unit, then the caller has to
somehow know that this metric wants to be in milliseconds, which seems to
defeat the purpose.
TBH, I don't understand this well...The use case I image is like this, e.g.,
```scala
allMetrics.put("metricA", new OurTimer(timeUnit = TimeUnit.SECONDS))
allMetrics.put("metricBMs", new OurTimer(timeUnit = TimeUnit.MILLISECONDS))
```
It's fine for the metric to omit the unit suffix. But, if the metric has the
unit suffix, then, it's the developer's responsibility to ensure the time unit
consistent between the metric and Timer.
> Then within YarnShuffleServiceMetrics#collectMetric:
>
> TimeUnit timeUnit;
> if (metric instanceof TimerWithUnits) {
> timeUnit = ((TimerWithUnits) metric).getTimeUnit();
> } else {
> timeUnit = TimeUnit.NANOSECONDS;
> }
> // do some conversion on the snapshot based on the unit above
And, with my understanding, the time unit should be used at all related
places inside the timer (Context, Histogram, etc). So, all APIs exposed by the
`Timer` would return the values based on the specified time unit directly.
Thus, we don't have to do the manual conversion after getting the return value.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
