Fix nanos to millis conversion for tests #11856
Conversation
matriv
force-pushed
the
mt/fix-nanos-millis
branch
from
bc81bf9
to
fe62114
Compare
| "ModCount: " | ||
| + modCounts[j] | ||
| + " Two fields took " | ||
| + (finish - start) / 1_000_000 |
There was a problem hiding this comment.
Maybe we could be less old-fashioned and just use TimeUnit.seconds(1).toNanos() here and in other places?....
There was a problem hiding this comment.
You mean maybe: TimeUnit.NANOSECONDS.toMillis(finish-start) ?
There was a problem hiding this comment.
Either way (constant to nanos or the value from nanos to millis).
matriv
force-pushed
the
mt/fix-nanos-millis
branch
2 times, most recently
from
aeec5aa
to
cc3e2e4
Compare
|
I pushed only for the wrong calculations, will let you know once I've converted all such divisions to use |
| @@ -4190,7 +4190,7 @@ private static Status.SoftDeletsStatus checkSoftDeletes( | |||
| } | |||
|
|
|||
| private static double nsToSec(long ns) { | |||
| return ns / 1000000000.0; | |||
| return ns / (double) TimeUnit.SECONDS.toNanos(1); | |||
There was a problem hiding this comment.
Heads up for such changes in production code, could this potentially add overhead?
There was a problem hiding this comment.
If you're concerned about it, move it to a constant? I think it'll be negligible.
There was a problem hiding this comment.
Any suggestion for a "central" place to put this?
There was a problem hiding this comment.
Nothing calls this gazillions of times, its only called once per "message" printed from checkindex, after it does a ton of work.
There was a problem hiding this comment.
Thx, Could you please check the PR?
Fix some wrong division to calculate millis from nanos in tests, after commit that removes System.currentTimeMillis() calls. Follows: fb40a43
Avoid error prone division with literals, in favour of TimeUnit conversion methods.
For consistency, use `ms` instead of `msec` or `millis` everywhere in the code base.
matriv
force-pushed
the
mt/fix-nanos-millis
branch
from
96a4c97
to
686132c
Compare
|
I think this is ready to be merged, sorry for the delay. One thing - could you add an appropriate lucene/CHANGES.txt entry? |
@dweiss Thanks a lot for taking care, apologies, I was not online for the last few days and missed your message. |
|
No worries. I added the missing entry manually. Thank you! |
Fix some wrong division to calculate millis from nanos in tests,
after commit that removes System.currentTimeMillis() calls.
Follows: fb40a43
Follows: Remove usages of System.currentTimeMillis() from tests #11749
Replace all sec, ms, ns calculations with TimeUnit conversion methods
Rename
msecandmillistomsto have consistency across the code base