Deprecation of LogRecord.getMillis in JDK9

[Daniel Fuchs]

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
>>>>>
>>>>
>>>
>>