HDDS-12060. Replace System.currentTimeMillis() with Time.monotonicNow() for duration calculation #8096
Conversation
…() for duration calculation
|
Hi, @ivandika3 |
| long currentTime = System.currentTimeMillis(); | ||
| long existingCount = processExistingDBRecords(currentTime, | ||
| long currentTime = Time.monotonicNow(); | ||
| long existingCount = processExistingDBRecords(Time.monotonicNow(), |
There was a problem hiding this comment.
Is this(change in line 150) also used for duration calculation? If I'm wrong, please correct me. Thanks!
There was a problem hiding this comment.
Hi, @peterxcli
Thanks for the review and comments. In the following line the start would be subtracted to estimate the duration, please take a look and tell me what do you think. Thanks!
LOG.debug("Container Health task thread took {} milliseconds to" +
" process {} existing database records.",
Time.monotonicNow() - start, existingCount);
There was a problem hiding this comment.
@chiacyu As @peterxcli pointed out, start already uses Time.monotonicNow. The currentTime is used for the UnhealthyContainers.inStateSince which will be displayed in Recon UI. So currentTime should still use System.currentTimeMillis .
Please kindly revert this change.
There was a problem hiding this comment.
Please revert both lines, not only currentTime.
There was a problem hiding this comment.
Thanks @chiacyu for taking this up. Left a comment regarding issue pointed out by @peterxcli
| long currentTime = System.currentTimeMillis(); | ||
| long existingCount = processExistingDBRecords(currentTime, | ||
| long currentTime = Time.monotonicNow(); | ||
| long existingCount = processExistingDBRecords(Time.monotonicNow(), |
There was a problem hiding this comment.
@chiacyu As @peterxcli pointed out, start already uses Time.monotonicNow. The currentTime is used for the UnhealthyContainers.inStateSince which will be displayed in Recon UI. So currentTime should still use System.currentTimeMillis .
Please kindly revert this change.
There was a problem hiding this comment.
Thanks @ivandika3 for the explanation and @chiacyu for updating the patch. Sorry for my previous comment is not clear enough. Left a new comment, please also take a look. Thanks!
| @@ -147,7 +147,7 @@ protected void runTask() throws Exception { | |||
| unhealthyContainerStateStatsMap); | |||
| long start = Time.monotonicNow(); | |||
| long currentTime = System.currentTimeMillis(); | |||
| long existingCount = processExistingDBRecords(currentTime, | |||
| long existingCount = processExistingDBRecords(Time.monotonicNow(), | |||
There was a problem hiding this comment.
After tracing down the code, I think the currentTime param of processExistingDBRecords is not used for calculate duration, it just store it with UnhealthyContainersRecord in recon db.
| long existingCount = processExistingDBRecords(Time.monotonicNow(), | |
| long existingCount = processExistingDBRecords(currentTime, |
There was a problem hiding this comment.
@chiacyu Please revert both of the lines, not only currentTime.
There was a problem hiding this comment.
@chiacyu , thanks a lot for working on this! The change looks good. Just have two questions inlined.
BTW, I suggest that this PR only changes the trivial replacements, i.e. when the time is used locally in a method for computing elapsed time. For the other more complicated that the time is stored in a class and it is used it multiple places, we can fix it in separated JIRAs. Such cases need to be reviewed carefully.
| @@ -123,7 +124,7 @@ public boolean operationalStateExpired() { | |||
| if (0 == opStateExpiryEpochSeconds) { | |||
| return false; | |||
| } | |||
| return System.currentTimeMillis() / 1000 >= opStateExpiryEpochSeconds; | |||
| return Time.monotonicNow() / 1000 >= opStateExpiryEpochSeconds; | |||
There was a problem hiding this comment.
Will NodeStatus be stored in disk? If it will, then we cannot use Time.monotonicNow() for opStateExpiryEpochSeconds since it does use unix epoch. It won't work after the server restart.
| @@ -147,7 +147,7 @@ protected void runTask() throws Exception { | |||
| unhealthyContainerStateStatsMap); | |||
| long start = Time.monotonicNow(); | |||
| long currentTime = System.currentTimeMillis(); | |||
There was a problem hiding this comment.
currentTime is still using System.currentTimeMillis(). It will be passed to ContainerHealthRecords.generateUnhealthyRecords(. Not sure if it should be changed to Time.monotonicNow().
What changes were proposed in this pull request?
A lot of duration calculations are still using
System.currentTimeMillis(). However, from the description of Time.now (which callsSystem.currentTimeMillis()Therefore, we should replace them with
Time.monotonicNow()/Time.monotonicNowNanos()to prevent the duration calculation being broken bysettimeofday.What is the link to the Apache JIRA
HDDS-12060
How was this patch tested?
Tested by CI