[GitHub] [spark] xkrogen edited a comment on pull request #33116: [SPARK-35259][SHUFFLE] Rename ExternalBlockHandler Timer variables to remove incorrect millis suffix

[GitBox <gi...@apache.org>]

xkrogen edited a comment on pull request #33116:
URL: https://github.com/apache/spark/pull/33116#issuecomment-880022892


   > I think we can force the `TimerWithMillisecondSnapshots` to receive a time-unit parameter to indicate which time-unit it exposes.
   
   I don't it makes sense to force callers to supply a time unit, since (a) we should adhere to the `Timer` interface, which already exposes the unitless methods, (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.
   
   If we want to go down a route like this, it's basically what I was proposing initially where `YarnShuffleServiceMetrics` does the conversion. The `Timer` interface doesn't provide any way to extract unit information, but we could either check if the metric name ends in "millis" (and if so, treat the unit as millis instead of nanos) or create a custom extension which has a method defining the units, like:
   ```
     static class TimerWithUnits extends Timer {
       private final TimeUnit timeUnit;
       TimerWithUnits(TimeUnit timeUnit) {
         this.timeUnit = timeUnit;
       }
       public TimeUnit getTimeUnit() {
         return timeUnit;
       }
     }
   ```
   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
   ```


-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org