Hi Stuart,
Thanks for the feedback!
On 30/11/15 23:53, Stuart Marks wrote:
Hi all,
Thanks for considering JEP 277 in this discussion. It's far from being
finalized at this point, but SUPERSEDED seems like the most likely of
the deprecation reasons from the proposal that would be applied here.
While it seems that getMillis() is merely a convenience method for
calling getInstant().toEpochMilli(), there are a couple subtle semantic
differences that might warrant consideration beyond the loss of
nanosecond resolution.
First, LogRecord now contains an Instant instead of a millis-since-epoch
value. Instant is explicitly intended to support points in time prior to
the epoch. It's possible for a LogRecord to contain such a time via
setInstant(). Thus, getMillis() can now return a negative number. This
isn't explicitly allowed or disallowed as far as I can see, but
historically millis-since-epoch values have always been non-negative. I
suspect that most code out there implicitly assumes this and would
unprepared to deal with negative return values.
This would only apply to code that explicitly used to call
LogRecord.setMillis() with negative values: the number of milliseconds
in LogRecord historically comes from System.currentTimeMillis() - and
using Instant is not changing that - since we are now
using Instant.now() - which underneath still ends up calling
System.currentTimeMillis() with an additional nano second adjsutment.
Second, Instant uses a long to store seconds before or after the epoch,
whereas a long millis-since-epoch value can only represent 1/1000th of
that range. Instant.toEpochMilli() throws an exception in such cases.
This behavior would show through to LogRecord.getMillis(). (It looks
like this limitation is also present in the serial form of LogRecord.)
Interesting. Maybe we should throw an IllegalArgumentException in
setInstant() if the instant we're given exceeds the capacity of
a long millis-since-epoch value. Or maybe it is acceptable to
let the exception be thrown at serialization time - and deal
with the issue in serialization when it is clear that there is a
requirement for representing such points in time in LogRecord
(dealing with this would mean altering the serial form
of LogRecord - and I'm not convinced we need to support such
extreme values). In any case, such an Instant would have to be
constructed from the calling code - and set explicitely through
setInstant() since Instant.now() is precisely constructed behind
the scene from a long millis-since-epoch + a nano second adjustment,
and therefore it should always be possible to retrofit it in those
two quantities.
My hunch is that it's exceedingly rare for a LogRecord contain a time
before 1970 or 292 million years in the future, but the API allows it,
so it could happen, and it should be specified.
Time before 1970 shouldn't be a problem - I think.
I mean that using Instant makes it now clear what a negative
value represents.
In what concerns 292 million years in the future, then
maybe we could schedule a RFE for Java 97333333.0 ;-)
Or more seriously wait until the Clock.systemClock()/Instant.now()
are able to return such value?
This doesn't imply that getMillis() should or shouldn't be deprecated.
The deciding factor here is whether you think it's important for
programmers to migrate away from this API. It may be that getMillis()
works just fine under all but the most extreme cases, so there's no
practical need for programmers to migrate away from it.
Right. Stephen has convinced me that removing the @Deprecated and
adding an @apiNote is the appropriate thing to do :-)
best regards,
-- daniel
s'marks
On 11/30/15 10:20 AM, Daniel Fuchs wrote:
On 30/11/15 18:43, Roger Riggs wrote:
Hi Daniel,
I think it makes sense to keep getMillis (and document it) as a
convenience method.
Thanks Roger, Jason, I logged
https://bugs.openjdk.java.net/browse/JDK-8144262
best regards,
-- daniel
Roger
On 11/30/2015 12:25 PM, Daniel Fuchs wrote:
On 30/11/15 18:04, Jason Mehrens wrote:
Hi Daniel,
When JDK-8072645 - java.util.logging should use java.time to get more
precise time stamps was commited the LogRecord.getMillis() method was
marked as deprecated with the reason "To get the full nanosecond
resolution event time, use getInstant". I can see marking
LogRecord.setMillis as deprecated since using that would be an
untended loss of precision. However, it seems excessive to deprecate
LogRecord.getMillis when it could be treated as a convenience method
that could simply note that if the caller wants nanosecond resolution
use getInstant. It would be extremely helpful compatibility wise to
have this undeprecated for libs that have support pre-Java 9. If it
can't be undeprecated what is the proper way to target support as low
as JDK7 but might end up executing on JDK9?
Hi Jason,
I see your point.
As you noted, the main reason for deprecating getMillis() is that we
actually wanted to deprecate setMillis().
If I remember well there was a discussion at the time around
whether calling setMillis() should or should not set the nano
second adjustment to 0.
We ended up adding an Instant field instead of simply adding
a new 'nanos' field adjustment, and then we deprecated
getMillis()/setMillis() in favor of getInstant()/setInstant().
That said - I agree that the only really problematic API here
is setMillis().
I wouldn't be opposed to 'undeprecate' getMillis() - I wonder
whether that would be a good use case for JEP 277 though.
(enhanced deprecation http://openjdk.java.net/jeps/277 )
Any other opinion?
best regards,
-- daniel
Thanks,
Jason
