RFR 8245302: Upgrade LogRecord to support long thread ids and remove its usage of ThreadLocal

[Rahul Yadav]

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