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.
