Hi Alan,
Thank you for the feedback.I have updated the webrev.
webrev :
http://cr.openjdk.java.net/~ryadav/webrev_8245302/webrev.00/index.html
- rahul
On 14/06/2020 14:22, Alan Bateman wrote:
On 10/06/2020 16:06, Rahul wrote:
Hello,
Request to have my fix reviewed for the issue:
JDK-8245302: Upgrade LogRecord to support long thread ids and
remove its usage of ThreadLocal.
java.util.logging.LogRecord is updated to support long thread ids
without ThreadLocal.
Before the update, thread id’s did not support long values,
ThreadLocal usage has been
Removed.
.
issue: https://bugs.openjdk.java.net/browse/JDK-8245302
webrev: http://cr.openjdk.java.net/~ryadav/webrev_8245302/index.html
csr: https://bugs.openjdk.java.net/browse/JDK-8247219
The addition of setLongThreadID/getLongThreadID, and deprecating the
old methods looks good. One suggestion is that setLongThreadID can
return the LogRecord (= this) to allow for method invocation chaining.
For the javadoc then it would be clearer to say "Returns the ..."
rather than "Get a ..." because a sequence of calls to getXXX has to
return the same thread ID.
LogRecord is serializable but it looks like like you've thought about
all the issue so I think this looks.
shortThreadID looks a bit strange in that I would have expected it
would just return tid when in the 0-Integer.MAX_VALUE range and
compute a negative value when it's larger than Integer.MAX_VALUE. It's
true that the old behavior was to switch to dense values when larger
than MAX_VALUE/2 but I can't imagine how anything could depend on
this. So not objecting to the approach, just pointing out that there
may be simpler alternatives that could be explored (if you haven't
done so already).
The overall test cover looks good. Can you provide clear instructions
in SerializeLogRecordTest on how to re-create the base64 string of the
serialized form? That is important for future maintainers.
There are several formatting/style nits in the patch but they aren't
important for now.
-Alan.
