DRILL-5230: Translation of millisecond duration into hours is incorrect #739
Conversation
|
+1 |
paul-rogers
left a comment
paul-rogers
left a comment
There was a problem hiding this comment.
Some minor changes suggested. In particular, move the very handy duration format logic to a utility so we can reuse it elsewhere.
| long milliSeconds = millis - TimeUnit.SECONDS.toMillis(TimeUnit.MILLISECONDS.toSeconds(millis)); | ||
| String formattedDuration = ""; | ||
| if (days >= 1) { | ||
| formattedDuration = days + "d" + hours + "h" + minutes + "m"; |
There was a problem hiding this comment.
Is this the display format? Maybe add some space:
5d 14h 2m
Instead of:
5d14h2m
There was a problem hiding this comment.
This display format is kept compact due to limited space within the cells of the table. Hence, we aim to keep only the first two major units. Hence, if something runs in hours, we report down to the minute, but not any more granular.
| } else if (minutes >= 1) { | ||
| formattedDuration = minutes + "m" + seconds + "s"; | ||
| } else { | ||
| formattedDuration = seconds + "." + milliSeconds + "s"; |
There was a problem hiding this comment.
The above won't work in general. If the time is 2 ms, you'll get an output of 0.2s, which is not quite right...
Here, consider using the very handy String.format method:
String.format("%.3f", seconds + milliSeconds / 1000.0 );
There was a problem hiding this comment.
Good catch. I'll update the test case as well and apply the String.format()
| } else { | ||
| formattedDuration = seconds + "." + milliSeconds + "s"; | ||
| } | ||
| return formattedDuration; |
There was a problem hiding this comment.
A nit, but the code is clearer if you do:
if (cond1) {
return expr1;
} else if (cond2) {
return expr2;
} else {
return expr3;
}
The above eliminates the temp variable and makes it very clear what is happening.
| * @param millis Duration in milliseconds | ||
| * @return Human-Readable Duration Time | ||
| */ | ||
| public String shortDurationFormat(long millis) { |
There was a problem hiding this comment.
Used elsewhere? If so, consider making static (so we don't need an instance.) If only used here, consider making private.
In fact, maybe move this out to a utility class. This logic can be used anywhere we want to display a duration such as in logs, in tests, etc.
| } | ||
| appendCell(timeFormat.format(new Date(p)), null); | ||
| String shortReadableDuration = shortDurationFormat(p); | ||
| appendCell(shortReadableDuration, link); |
There was a problem hiding this comment.
In Drill, we like brevity:
appendCell(shortDurationFormat(p), link);
kkhatua
force-pushed
the
DRILL-5230
branch
6 times, most recently
from
e837c62 to
94c0efb
Compare
|
@paul-rogers , @sudheeshkatkam |
paul-rogers
left a comment
paul-rogers
left a comment
There was a problem hiding this comment.
Thanks for moving the duration formatting to an easy-to-reuse utility!
One minor comment, one minor revision to the code that does the two format styles.
| * @return Human-Readable Elapsed Time | ||
| */ | ||
| public static String getPrettyDuration(long startTimeMillis, long endTimeMillis) { | ||
| public static String getPrettyDuration(long startTimeMillis, long endTimeMillis, DurationFormat format) { |
There was a problem hiding this comment.
It is often cleaner to just have two methods, rather than one method with a "command". Since we need to split out the data into a bunch of fields, this can be done by another method that creates a structure. Then, since you've created the structure, it might as well be the class that does the format, and offer two format methods: compact and verbose.
|
|
||
| /** | ||
| * Duration format definition | ||
| * @author kkhatua |
There was a problem hiding this comment.
Drill practice seems to be to omit the author. Many IDE's helpfully add this, but you can turn off that option.
There was a problem hiding this comment.
My bad. Will fix that.
kkhatua
force-pushed
the
DRILL-5230
branch
2 times, most recently
from
3a17402 to
e185c6d
Compare
Fixed invalid representation of readable elapsed time using `TimeUnit` class in JDK. e.g. 4545 sec is now correctly translated as `1h15m` instead of `17h15m`
|
@paul-rogers Created a new SimpleDurationFormat class. We can expand to have more formats, or reimplement on lines of SimpleDateFormat by passing format strings in the future. Hope this helps. |
paul-rogers
left a comment
paul-rogers
left a comment
There was a problem hiding this comment.
Thanks for pulling the duration format into a new class. Very helpful.
LGTM
+1
|
This closes PR #739 |
3 participants
Fixed invalid representation of readable elapsed time using
TimeUnitclass in JDK.e.g. 4545 sec is now correctly translated as
1h15minstead of17h15mTestCase has been added