[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR(S): 6545295: TEST BUG: The test HatHeapDump1Test uses wrong path in exec call.
From: Yekaterina Kantserova <yekaterina.kantserova () oracle ! com>
Date: 2014-06-16 9:05:45
Message-ID: 539EB369.3020009 () oracle ! com
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
Oh, great! Could you please be my sponsor? The patch is attached to this
mail.
// Katja
On 06/16/2014 10:45 AM, Staffan Larsen wrote:
> Katja,
>
> The policy for reviews in the jdk repo is different from the hotspot
> repo. For the jdk it is ok with a single review.
>
> /Staffan
>
>
> On 16 jun 2014, at 09:59, Yekaterina Kantserova
> <yekaterina.kantserova@oracle.com
> <mailto:yekaterina.kantserova@oracle.com>> wrote:
>
>> Hi,
>>
>> could I get one more review please?
>>
>> Thanks,
>> Katja
>>
>>
>> On 06/13/2014 02:23 PM, Staffan Larsen wrote:
>>> Looks good!
>>>
>>> Thanks,
>>> /Staffan
>>>
>>> On 13 jun 2014, at 14:15, Yekaterina Kantserova
>>> <yekaterina.kantserova@oracle.com
>>> <mailto:yekaterina.kantserova@oracle.com>> wrote:
>>>
>>>> Staffan, thanks for your review!
>>>>
>>>> The new webrev can be found
>>>> here:http://cr.openjdk.java.net/~ykantser/6545295/webrev.01/
>>>> <http://cr.openjdk.java.net/%7Eykantser/6545295/webrev.01/>
>>>>
>>>> // Katja
>>>>
>>>> On 06/13/2014 01:44 PM, Staffan Larsen wrote:
>>>>> Katja,
>>>>>
>>>>> Great to see this simplification! A couple of comments:
>>>>>
>>>>> - I don’t think you need to launch jhat with -XX:+UsePerfData - no
>>>>> one is attaching or reading data from the process.
>>>>>
>>>>> - The static processBuilder field seems to be mis-used. Why not
>>>>> use different ProcessBuilder objects for the HelloWorld and jhat?
>>>>>
>>>>> - For dumpFile.deleteOnExit() to be useful you should add it as
>>>>> early as possible (line 48 perhaps?). If it is the last line in
>>>>> the test you can just as well do dumpFile.delete().
>>>>
>>>> Of cause it should be dumpFile.delete()! - dumpFile.deleteOnExit()
>>>> has crept in by mistake.
>>>>
>>>>>
>>>>> /S
>>>>>
>>>>>
>>>>> On 13 jun 2014, at 13:25, Yekaterina Kantserova
>>>>> <yekaterina.kantserova@oracle.com
>>>>> <mailto:yekaterina.kantserova@oracle.com>> wrote:
>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> Could I please have a review of this fix.
>>>>>>
>>>>>> webrev: http://cr.openjdk.java.net/~ykantser/6545295
>>>>>> <http://cr.openjdk.java.net/%7Eykantser/6545295>
>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-6545295
>>>>>>
>>>>>> The test has been re-written using the testlibrary's
>>>>>> functionality. It's verified internally on all basic platforms.
>>>>>>
>>>>>> Thanks,
>>>>>> Katja
>>>
>>
>
[Attachment #5 (text/html)]
<html>
<head>
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">Oh, great! Could you please be my
sponsor? The patch is attached to this mail.<br>
<br>
// Katja<br>
<br>
<br>
<br>
On 06/16/2014 10:45 AM, Staffan Larsen wrote:<br>
</div>
<blockquote
cite="mid:BBEBC1C8-26B4-4E4C-B922-F270F8CB1EA2@oracle.com"
type="cite">
<meta http-equiv="Content-Type" content="text/html;
charset=windows-1252">
Katja,
<div><br>
</div>
<div>The policy for reviews in the jdk repo is different from the
hotspot repo. For the jdk it is ok with a single review.</div>
<div><br>
</div>
<div>/Staffan</div>
<div><br>
</div>
<div><br>
<div>
<div>On 16 jun 2014, at 09:59, Yekaterina Kantserova <<a
moz-do-not-send="true"
href="mailto:yekaterina.kantserova@oracle.com">yekaterina.kantserova@oracle.com</a>>
wrote:</div>
<br class="Apple-interchange-newline">
<blockquote type="cite">
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
<div text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">Hi,<br>
<br>
could I get one more review please?<br>
<br>
Thanks,<br>
Katja<br>
<br>
<br>
On 06/13/2014 02:23 PM, Staffan Larsen wrote:<br>
</div>
<blockquote
cite="mid:63AA910B-7ABF-4527-9130-72207A70ED3F@oracle.com"
type="cite">
<meta http-equiv="Content-Type" content="text/html;
charset=windows-1252">
<div>Looks good!</div>
<div><br>
</div>
<div>Thanks,</div>
<div>/Staffan</div>
<div><br>
</div>
<div>
<div>On 13 jun 2014, at 14:15, Yekaterina Kantserova
<<a moz-do-not-send="true"
\
href="mailto:yekaterina.kantserova@oracle.com">yekaterina.kantserova@oracle.com</a>>
wrote:</div>
<br class="Apple-interchange-newline">
<blockquote type="cite">
<div style="font-size: 16px; font-style: normal;
font-variant: normal; font-weight: normal;
letter-spacing: normal; line-height: normal;
orphans: auto; text-align: start; text-indent:
0px; text-transform: none; white-space: normal;
widows: auto; word-spacing: 0px;
-webkit-text-stroke-width: 0px;">Staffan, thanks
for your review!<br>
<br>
The new webrev can be found here:<span
class="Apple-converted-space"> </span><a
moz-do-not-send="true"
\
href="http://cr.openjdk.java.net/%7Eykantser/6545295/webrev.01/">http://cr.openjdk.java.net/~ykantser/6545295/webrev.01/</a><br>
<br>
// Katja<br>
<br>
On 06/13/2014 01:44 PM, Staffan Larsen wrote:<br>
<blockquote type="cite">Katja,<br>
<br>
Great to see this simplification! A couple of
comments:<br>
<br>
- I don’t think you need to launch jhat with
-XX:+UsePerfData - no one is attaching or
reading data from the process.<br>
<br>
- The static processBuilder field seems to be
mis-used. Why not use different ProcessBuilder
objects for the HelloWorld and jhat?<br>
<br>
- For dumpFile.deleteOnExit() to be useful you
should add it as early as possible (line 48
perhaps?). If it is the last line in the test
you can just as well do dumpFile.delete().<br>
</blockquote>
<br>
Of cause it should be dumpFile.delete()! -
dumpFile.deleteOnExit() has crept in by mistake.<br>
<br>
<blockquote type="cite"><br>
/S<br>
<br>
<br>
On 13 jun 2014, at 13:25, Yekaterina Kantserova
<<a moz-do-not-send="true"
\
href="mailto:yekaterina.kantserova@oracle.com">yekaterina.kantserova@oracle.com</a>>
wrote:<br>
<br>
<blockquote type="cite">Hi,<br>
<br>
Could I please have a review of this fix.<br>
<br>
webrev: <a moz-do-not-send="true"
\
href="http://cr.openjdk.java.net/%7Eykantser/6545295">http://cr.openjdk.java.net/~ykantser/6545295</a><br>
bug: <a moz-do-not-send="true"
\
href="https://bugs.openjdk.java.net/browse/JDK-6545295">https://bugs.openjdk.java.net/browse/JDK-6545295</a><br>
<br>
The test has been re-written using the
testlibrary's functionality. It's verified
internally on all basic platforms.<br>
<br>
Thanks,<br>
Katja</blockquote>
</blockquote>
</div>
</blockquote>
</div>
<br>
</blockquote>
<br>
</div>
</blockquote>
</div>
<br>
</div>
</blockquote>
<br>
</body>
</html>
["6545295.1.patch.zip" (application/zip)]
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic