RFR: JDK-8163185: jhsdb jstack cannot work with normal mode

[David Holmes]

On 8/08/2016 11:17 PM, Yasumasa Suenaga wrote:
> Hi David,
>
>> It is a better fix in my opinion but still changes the behaviour of
>> jhsdb - or is that not an issue? Is jhsdb new with 9?
>
> Can I list you as a reviewer?
> This change is agreed by Dmitry. So I think we can move forward it.

Yes to both. :)

Thanks,
David

>
> Thanks,
>
> Yasumasa
>
>
> On 2016/08/05 20:38, David Holmes wrote:
>> On 5/08/2016 8:55 PM, Yasumasa Suenaga wrote:
>>> Hi David,
>>>
>>> Thank you for your comment.
>>>
>>> For not to break current behavior, I think we should not change c'tor of
>>> JStack.
>>> I uploaded new webrev:
>>>
>>>   http://cr.openjdk.java.net/~ysuenaga/JDK-8163185/webrev.01/
>>>
>>> David, is it okay?
>>
>> It is a better fix in my opinion but still changes the behaviour of
>> jhsdb - or is that not an issue? Is jhsdb new with 9?
>>
>> Thanks,
>> David
>>
>>> Dmitry, could you review again?
>>>
>>>
>>> Thanks,
>>>
>>> Yasumasa
>>>
>>>
>>> On 2016/08/05 14:49, David Holmes wrote:
>>>> On 5/08/2016 12:24 AM, Yasumasa Suenaga wrote:
>>>>> Hi all,
>>>>>
>>>>> This review request is related to [1].
>>>>>
>>>>> "jhsdb jstack" should work as normal mode without being added --mixed
>>>>> option.
>>>>> However, this command always works as mixed mode.
>>>>
>>>> So it seems to me the JStack class has a very poorly designed API and
>>>> command-line interface. It appears that jstack wants to default to
>>>> mixedMode and concurrentLocks, but provides no means to disable those
>>>> defaults. Given there is no way to turn those settings off from the
>>>> commad-line then the default should be that they are off! Then you
>>>> would not need current proposed change.
>>>>
>>>> That said the proposed fix effectively disables those defaults, which
>>>> seems to me to be a change in behaviour.
>>>>
>>>> I can't see a way to fix this without breaking existing behaviour
>>>> somewhere. In which case I would change the way the JStack instance is
>>>> initialized by default ie:
>>>>
>>>> public JStack() {
>>>>     this(true, true);
>>>> }
>>>>
>>>> becomes:
>>>>
>>>> public JStack() {
>>>>     this(false, false);
>>>> }
>>>>
>>>> Or it may be better to handle this on the jhsdb side and change this:
>>>>
>>>>        JStack.main(newArgs.toArray(new String[newArgs.size()]));
>>>> to
>>>>
>>>>        JStack js = new jstack(false,false);
>>>>        js.runWithArgs(newArgs.toArray(new String[newArgs.size()]));
>>>>
>>>>
>>>> Cheers,
>>>> David
>>>>
>>>>> So I uploaded webrev for this issue. Could you review it?
>>>>>
>>>>>   http://cr.openjdk.java.net/~ysuenaga/JDK-8163185/webrev.00/
>>>>>
>>>>>
>>>>> I'm jdk 9 reviewer (ysuenaga), but I cannot access JPRT.
>>>>> So I need a sponsor.
>>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Yasumasa
>>>>>
>>>>>
>>>>> [1]
>>>>> http://mail.openjdk.java.net/pipermail/serviceability-dev/2016-August/020087.html
>>>>>
>>>>>
>>>>>