I think #3 or a variation is best. Clearly, #1 is inconsistent and simply
documenting it (#2) isn't much better.
I'd recommend making setInstant() be more explicit about the range of Instant
values that are allowed, namely those created from Instant.ofEpochMilli(long),
which allows +/- 292 million years from the epoch. Otherwise the reader is
forced to try to understand when Instant.toEpochMilli() throws the exception.
Now that we've constrained the range of instants that a LogRecord can contain,
is it still necessary for setMillis(long) to be deprecated? Every value is
valid. It can set (almost) the full range of Instant values, just not with
nanosecond resolution. If you don't care about nanosecond resolution, this seems
like a perfectly fine method to use.
s'marks
On 12/1/15 10:48 AM, Daniel Fuchs wrote:
Hi Jason, Stuart,
Here is a potential fix for the issue:
http://cr.openjdk.java.net/~dfuchs/webrev_8144262/webrev.00/src/java.logging/share/classes/java/util/logging/LogRecord.java.frames.html
http://cr.openjdk.java.net/~dfuchs/webrev_8144262/specdiff-logging/java/util/logging/LogRecord.html
As Stuart noted, java.time.Instant has a greater range than what can
be constructed from a long milliseconds-since-epoch + a nano-time
adjustment. This does not apply to instants returned by the system
clock, since those are constructed precisely from such long
milliseconds-since-epoch + nano-time adjustment.
However - someone could conceivably construct such an Instant
and pass it to a LogRecord. If that happens, then LogRecord.getMillis()
could potentially throw an undocumented ArithmeticException.
So we have at least 3 possibilities:
1. do nothing
2. document that getMillis() can throw ArithmeticException, with the
additional consequence that serializing a LogRecord thus constructed
would also throw an ArithmeticException.
3. modify setInstant() to validate that the instant will fit in a
long milliseconds-since-epoch.
The above patch implements option 3 (which currently has my
preference). Is that the best solution?
I would very much like to hear your opinion.
If it seems like the best then I'll add a unit test, send an RFR, and
do the paper work for the spec change...
best regards, and thanks for all the valuable feedback!
-- daniel
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?
Thanks,
Jason
