- 
                Notifications
    You must be signed in to change notification settings 
- Fork 2k
Fixes #3515 - Review Uptime. #13648
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
Fixes #3515 - Review Uptime. #13648
Conversation
* Deprecated for removal Uptime. * Remove its usages, replaced by using Instant in Server. Signed-off-by: Simone Bordet <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are merge conflicts
…1.x/3515-remove-uptime # Conflicts: # jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java
Signed-off-by: Simone Bordet <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved, but it seems like we would have an insignificant bug thru a DST change.
If so, would a negative value cause any bugs?
| */ | ||
| public long getUptimeMillis() | ||
| { | ||
| return Duration.between(startupInstant, Instant.now()).toMillis(); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't this calculation be wrong twice a year during DST changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but in order to fix this we would need to know the ZoneId.
We could use ZoneId.systemDefault(), and modernize Server to have a ZonedDateTime as startup "time", rather than just Instant.
If users want to configure a different timezone for the JVM, they can use the system property user.timezone.
Now the startupTime is a ZonedDateTime, which takes into account daylight savings. Signed-off-by: Simone Bordet <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM if we accept the tiny breakage.
| } | ||
|  | ||
| @ManagedAttribute("The UTC startup instant") | ||
| @ManagedAttribute("The startup date time in the system timezone") | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a somewhat breaking change, although not necessarily a bad one.
Signed-off-by: Simone Bordet <[email protected]>
Uh oh!
There was an error while loading. Please reload this page.