jmx-dev Review Request: 7195779 javax/management/remote/mandatory/threads/ExecutorTest.java fail intermittently

[Jaroslav Bachorik]

I have updated the patch to reflect Alan's remarks. The webrev is at the
same location - github takes care of versioning ...

-JB-

On 10/04/2012 05:56 PM, Jaroslav Bachorik wrote:
> On Thu 04 Oct 2012 05:42:15 PM CEST, Alan Bateman wrote:
>> On 04/10/2012 16:28, Jaroslav Bachorik wrote:
>>> :
>>> This is a follow-up. I've prepared the patch and put it on github -
>>> https://github.com/jbachorik/openjdk-patches/tree/master/webrevs/7195779
>>>
>>> I wonder who else should be included in the review process since I am
>>> changing the IIOP generator code. Also, I didn't find any tests in the
>>> corba repository. Which test suite is appropriate to run after changing
>>> the corba related code?
>>>
>>> -JB-
>>>
>> I don't mind being reviewer and sponsor for this. Also cc'ing Sean as
>> he is one of the maintainers of the corba code. I don't think the
> 
> Thanks.
> 
>> corba tests are in OpenJDK, at least I don't think Oracle has
>> contributed its tests for this area.
>>
>> I think your change looks okay and I assume you've at least run the
>> JMX tests that use RMI-IIOP to verify that the intermittent NPE is
>> gone and those tests now pass reliably.
> 
> Yes, I ran those. Especially on the test machine yielding the biggest 
> ration of failed tests previously. Now they all pass.
> 
>>
>> Minor comment but if I were doing this myself then I probably would
>> have added this instead:
>> p.pln(getName(theType) + " target = this.target;");
>>
>> You'll see lots of examples of this in the core libs and j.u.c.
> 
> No problem. If it is a convention I will stick to it (I've used a 
> different variable name to prevent confusion about what is a field and 
> what is a local variable).
> 
>>
>> Also as target is now volatile then I'm not sure why you synchronized
>> around target=null, perhaps there is other code generated in the tie
>> class that I don't see?
> 
> The synchronization should not be there. It just escaped my purging 
> when I've exchanged the synchronized access for volatile.
> 
> -JB-
> 
>>
>> Otherwise it's great to get issue finally resolved.
>>
>> -Alan.
>>
>>
> 
>