Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HDDS-7911. Replace Time.now() with Instant.now() #7390

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ptlrs
Copy link
Contributor

@ptlrs ptlrs commented Nov 4, 2024

What changes were proposed in this pull request?

Transition from Time.now() to Instant.now().toEpochMilli()

Please describe your PR in detail:
Ozone currently uses Hadoop util's Time.now() calls which internally calls System.currentTimeMillis() to obtain Unix epoch milliseconds.

The System.currentTimeMillis() javadoc states:

public static long currentTimeMillis()
Returns:
the difference, measured in milliseconds, between the current time and midnight, January 1, 1970 UTC.

The current time can be affected by:

  • Manual changes to the system time.
  • Automatic adjustments when crossing time zones.
  • Daylight saving time changes.

This can result in time discrepancy in Ozone.

The alternative is to use Instant.now() which is not affected by changes to system time zones.

In this PR:

  • Transition from Time.now() to Instant.now().toEpochMilli()
  • Replace Instant.ofEpochMilli(Time.now()) with Instant.now()

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-7911

How was this patch tested?

CI:
https://github.com/ptlrs/ozone/actions/runs/11659413284
https://github.com/ptlrs/ozone/actions/runs/11899696799

@ptlrs ptlrs marked this pull request as ready for review November 4, 2024 18:08
@ayushtkn
Copy link
Member

ayushtkn commented Nov 4, 2024

depends upon the use case, if we are measuring the during b/w two events then we should ideally use Time.monotonicNow(), but I doubt if Instant.now() is helping much..

Regarding DayLight changes, are you sure System.currentMilliseconds() behaves differently ?
image

Both Instant.now() & System.currentMilliseconds are impacted by setTimeofday kind of operation AFAIK, I double checked as well

image

Was checking further, I think this might have performance penalties as well
image

so, if the above is true, & we need to do this then I believe we should be selective about the places, once we establish Instant.now() is better & helping in X use cases at Y places

@adoroszlai adoroszlai marked this pull request as draft November 5, 2024 08:48
…-now

# Conflicts:
#	hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestKeyManagerImpl.java
#	hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotCreateRequest.java
#	hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotRenameRequest.java
@ptlrs
Copy link
Contributor Author

ptlrs commented Nov 19, 2024

Thank you for the review @ayushtkn.

As you mentioned, Time.monotonicNow() or Java's nanotime is not an option here as we are not measuring elapsed time.

The other consideration is that we want to reduce the dependency on the Hadoop library.

On the performance side, it may be too early to optimize but here are some rough numbers via JMH for calling System.currentTimeMillis and Instant.now one million times on my machine.

The time taken to call either of the methods will not be the dominant factor at Ozone's scale.

Benchmark                                Mode  Cnt   Score   Error   Units
TimeBenchmark.measureCurrentTimeMillis  thrpt        0.075          ops/ms
TimeBenchmark.measureInstantNow         thrpt        0.051          ops/ms
TimeBenchmark.measureCurrentTimeMillis   avgt       13.334           ms/op
TimeBenchmark.measureInstantNow          avgt       19.823           ms/op

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants