You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by Laszlo Hornyak <la...@gmail.com> on 2013/06/18 22:12:01 UTC
Review Request: ProcessUtil cleanup
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11942/
-----------------------------------------------------------
Review request for cloudstack.
Description
-------
- possible resource leak closed
- file content read uses now commons-lang FileUtils
- Added unit tests
Diffs
-----
utils/src/com/cloud/utils/ProcessUtil.java c9fdf35
utils/test/com/cloud/utils/ProcessUtilTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/11942/diff/
Testing
-------
test included
Thanks,
Laszlo Hornyak
Re: Review Request 11942: ProcessUtil cleanup
Posted by "Jenkins Cloudstack.org" <hu...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11942/#review23615
-----------------------------------------------------------
Review 11942 PASSED the build test
The url of build cloudstack-master-with-patch #4 is : http://jenkins.cloudstack.org/job/cloudstack-master-with-patch/4/
- Jenkins Cloudstack.org
On July 20, 2013, 8:01 p.m., Laszlo Hornyak wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11942/
> -----------------------------------------------------------
>
> (Updated July 20, 2013, 8:01 p.m.)
>
>
> Review request for cloudstack.
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> - possible resource leak closed
> - file content read uses now commons-lang FileUtils
> - Added unit tests
>
>
> Diffs
> -----
>
> utils/src/com/cloud/utils/ProcessUtil.java c9fdf35
> utils/test/com/cloud/utils/ProcessUtilTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/11942/diff/
>
>
> Testing
> -------
>
> test included
>
>
> Thanks,
>
> Laszlo Hornyak
>
>
Re: Review Request 11942: ProcessUtil cleanup
Posted by Laszlo Hornyak <la...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11942/#review25005
-----------------------------------------------------------
Hi Reviewers,
Any feedback on this patch? :)
- Laszlo Hornyak
On July 24, 2013, 4:42 a.m., Laszlo Hornyak wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11942/
> -----------------------------------------------------------
>
> (Updated July 24, 2013, 4:42 a.m.)
>
>
> Review request for cloudstack, Frank Zhang and John Burwell.
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> - possible resource leak closed
> - file content read uses now commons-lang FileUtils
> - Added unit tests
>
>
> Diffs
> -----
>
> utils/src/com/cloud/utils/ProcessUtil.java c9fdf35
> utils/test/com/cloud/utils/ProcessUtilTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/11942/diff/
>
>
> Testing
> -------
>
> test included
>
>
> Thanks,
>
> Laszlo Hornyak
>
>
Re: Review Request 11942: ProcessUtil cleanup
Posted by John Burwell <jb...@basho.com>.
Laszlo,
I was only concerned about the lack of asserts in the original test case. That has been updated, so it looks good to me now.
Thanks,
-John
On Aug 12, 2013, at 5:43 PM, "Frank Zhang" <fr...@citrix.com> wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11942/#review25036
> -----------------------------------------------------------
>
> Ship it!
>
>
> This is very straightforward patch. Java filesystem api is known as unnecessary complex, we should take advantage of Apache common-io as much as possible.
> shipped in 4.2 and master
>
> - Frank Zhang
>
>
> On July 24, 2013, 4:42 a.m., Laszlo Hornyak wrote:
>>
>> -----------------------------------------------------------
>> This is an automatically generated e-mail. To reply, visit:
>> https://reviews.apache.org/r/11942/
>> -----------------------------------------------------------
>>
>> (Updated July 24, 2013, 4:42 a.m.)
>>
>>
>> Review request for cloudstack, Frank Zhang and John Burwell.
>>
>>
>> Repository: cloudstack-git
>>
>>
>> Description
>> -------
>>
>> - possible resource leak closed
>> - file content read uses now commons-lang FileUtils
>> - Added unit tests
>>
>>
>> Diffs
>> -----
>>
>> utils/src/com/cloud/utils/ProcessUtil.java c9fdf35
>> utils/test/com/cloud/utils/ProcessUtilTest.java PRE-CREATION
>>
>> Diff: https://reviews.apache.org/r/11942/diff/
>>
>>
>> Testing
>> -------
>>
>> test included
>>
>>
>> Thanks,
>>
>> Laszlo Hornyak
>>
>>
>
RE: Review Request 11942: ProcessUtil cleanup
Posted by Frank Zhang <Fr...@citrix.com>.
I reverted it from 4.2. now is adding the Apache header to master
> -----Original Message-----
> From: Animesh Chaturvedi
> Sent: Monday, August 12, 2013 5:56 PM
> To: dev@cloudstack.apache.org; Frank Zhang; John Burwell
> Cc: Laszlo Hornyak
> Subject: RE: Review Request 11942: ProcessUtil cleanup
>
> ProcessUtilTest is missing Apache License Header and failing rat-test
>
> > -----Original Message-----
> > From: Animesh Chaturvedi [mailto:animesh.chaturvedi@citrix.com]
> > Sent: Monday, August 12, 2013 5:40 PM
> > To: Frank Zhang; dev@cloudstack.apache.org; John Burwell
> > Cc: Laszlo Hornyak
> > Subject: RE: Review Request 11942: ProcessUtil cleanup
> >
> > We should keep it for master
> >
> > > -----Original Message-----
> > > From: Frank Zhang
> > > Sent: Monday, August 12, 2013 5:40 PM
> > > To: Animesh Chaturvedi; dev@cloudstack.apache.org; John Burwell
> > > Cc: Laszlo Hornyak
> > > Subject: RE: Review Request 11942: ProcessUtil cleanup
> > >
> > > Emm. Actually it's not a blocker or even critical for 4.2.
> > > Though I thought it's not harm, if needs I can revert it from 4.2
> > > but still keep it in master.
> > >
> > > > -----Original Message-----
> > > > From: Animesh Chaturvedi
> > > > Sent: Monday, August 12, 2013 5:37 PM
> > > > To: dev@cloudstack.apache.org; Frank Zhang; John Burwell
> > > > Cc: Laszlo Hornyak
> > > > Subject: RE: Review Request 11942: ProcessUtil cleanup
> > > >
> > > > Is this really needed for 4.2? 4.2 is in limited updates and only
> > > > blocker/critical bug fixes and doc and test updates are allowed.
> > > > All commits to 4.2 should have associated BUG id.
> > > >
> > > > > -----Original Message-----
> > > > > From: Frank Zhang [mailto:noreply@reviews.apache.org] On Behalf
> > > > > Of Frank Zhang
> > > > > Sent: Monday, August 12, 2013 2:43 PM
> > > > > To: John Burwell; Frank Zhang
> > > > > Cc: Laszlo Hornyak; cloudstack
> > > > > Subject: Re: Review Request 11942: ProcessUtil cleanup
> > > > >
> > > > >
> > > > > -----------------------------------------------------------
> > > > > This is an automatically generated e-mail. To reply, visit:
> > > > > https://reviews.apache.org/r/11942/#review25036
> > > > > -----------------------------------------------------------
> > > > >
> > > > > Ship it!
> > > > >
> > > > >
> > > > > This is very straightforward patch. Java filesystem api is known
> > > > > as unnecessary complex, we should take advantage of Apache
> > > > > common-io as much as possible.
> > > > > shipped in 4.2 and master
> > > > >
> > > > > - Frank Zhang
> > > > >
> > > > >
> > > > > On July 24, 2013, 4:42 a.m., Laszlo Hornyak wrote:
> > > > > >
> > > > > > -----------------------------------------------------------
> > > > > > This is an automatically generated e-mail. To reply, visit:
> > > > > > https://reviews.apache.org/r/11942/
> > > > > > -----------------------------------------------------------
> > > > > >
> > > > > > (Updated July 24, 2013, 4:42 a.m.)
> > > > > >
> > > > > >
> > > > > > Review request for cloudstack, Frank Zhang and John Burwell.
> > > > > >
> > > > > >
> > > > > > Repository: cloudstack-git
> > > > > >
> > > > > >
> > > > > > Description
> > > > > > -------
> > > > > >
> > > > > > - possible resource leak closed
> > > > > > - file content read uses now commons-lang FileUtils
> > > > > > - Added unit tests
> > > > > >
> > > > > >
> > > > > > Diffs
> > > > > > -----
> > > > > >
> > > > > > utils/src/com/cloud/utils/ProcessUtil.java c9fdf35
> > > > > > utils/test/com/cloud/utils/ProcessUtilTest.java PRE-CREATION
> > > > > >
> > > > > > Diff: https://reviews.apache.org/r/11942/diff/
> > > > > >
> > > > > >
> > > > > > Testing
> > > > > > -------
> > > > > >
> > > > > > test included
> > > > > >
> > > > > >
> > > > > > Thanks,
> > > > > >
> > > > > > Laszlo Hornyak
> > > > > >
> > > > > >
RE: Review Request 11942: ProcessUtil cleanup
Posted by Animesh Chaturvedi <an...@citrix.com>.
ProcessUtilTest is missing Apache License Header and failing rat-test
> -----Original Message-----
> From: Animesh Chaturvedi [mailto:animesh.chaturvedi@citrix.com]
> Sent: Monday, August 12, 2013 5:40 PM
> To: Frank Zhang; dev@cloudstack.apache.org; John Burwell
> Cc: Laszlo Hornyak
> Subject: RE: Review Request 11942: ProcessUtil cleanup
>
> We should keep it for master
>
> > -----Original Message-----
> > From: Frank Zhang
> > Sent: Monday, August 12, 2013 5:40 PM
> > To: Animesh Chaturvedi; dev@cloudstack.apache.org; John Burwell
> > Cc: Laszlo Hornyak
> > Subject: RE: Review Request 11942: ProcessUtil cleanup
> >
> > Emm. Actually it's not a blocker or even critical for 4.2.
> > Though I thought it's not harm, if needs I can revert it from 4.2 but
> > still keep it in master.
> >
> > > -----Original Message-----
> > > From: Animesh Chaturvedi
> > > Sent: Monday, August 12, 2013 5:37 PM
> > > To: dev@cloudstack.apache.org; Frank Zhang; John Burwell
> > > Cc: Laszlo Hornyak
> > > Subject: RE: Review Request 11942: ProcessUtil cleanup
> > >
> > > Is this really needed for 4.2? 4.2 is in limited updates and only
> > > blocker/critical bug fixes and doc and test updates are allowed. All
> > > commits to 4.2 should have associated BUG id.
> > >
> > > > -----Original Message-----
> > > > From: Frank Zhang [mailto:noreply@reviews.apache.org] On Behalf Of
> > > > Frank Zhang
> > > > Sent: Monday, August 12, 2013 2:43 PM
> > > > To: John Burwell; Frank Zhang
> > > > Cc: Laszlo Hornyak; cloudstack
> > > > Subject: Re: Review Request 11942: ProcessUtil cleanup
> > > >
> > > >
> > > > -----------------------------------------------------------
> > > > This is an automatically generated e-mail. To reply, visit:
> > > > https://reviews.apache.org/r/11942/#review25036
> > > > -----------------------------------------------------------
> > > >
> > > > Ship it!
> > > >
> > > >
> > > > This is very straightforward patch. Java filesystem api is known
> > > > as unnecessary complex, we should take advantage of Apache
> > > > common-io as much as possible.
> > > > shipped in 4.2 and master
> > > >
> > > > - Frank Zhang
> > > >
> > > >
> > > > On July 24, 2013, 4:42 a.m., Laszlo Hornyak wrote:
> > > > >
> > > > > -----------------------------------------------------------
> > > > > This is an automatically generated e-mail. To reply, visit:
> > > > > https://reviews.apache.org/r/11942/
> > > > > -----------------------------------------------------------
> > > > >
> > > > > (Updated July 24, 2013, 4:42 a.m.)
> > > > >
> > > > >
> > > > > Review request for cloudstack, Frank Zhang and John Burwell.
> > > > >
> > > > >
> > > > > Repository: cloudstack-git
> > > > >
> > > > >
> > > > > Description
> > > > > -------
> > > > >
> > > > > - possible resource leak closed
> > > > > - file content read uses now commons-lang FileUtils
> > > > > - Added unit tests
> > > > >
> > > > >
> > > > > Diffs
> > > > > -----
> > > > >
> > > > > utils/src/com/cloud/utils/ProcessUtil.java c9fdf35
> > > > > utils/test/com/cloud/utils/ProcessUtilTest.java PRE-CREATION
> > > > >
> > > > > Diff: https://reviews.apache.org/r/11942/diff/
> > > > >
> > > > >
> > > > > Testing
> > > > > -------
> > > > >
> > > > > test included
> > > > >
> > > > >
> > > > > Thanks,
> > > > >
> > > > > Laszlo Hornyak
> > > > >
> > > > >
RE: Review Request 11942: ProcessUtil cleanup
Posted by Animesh Chaturvedi <an...@citrix.com>.
We should keep it for master
> -----Original Message-----
> From: Frank Zhang
> Sent: Monday, August 12, 2013 5:40 PM
> To: Animesh Chaturvedi; dev@cloudstack.apache.org; John Burwell
> Cc: Laszlo Hornyak
> Subject: RE: Review Request 11942: ProcessUtil cleanup
>
> Emm. Actually it's not a blocker or even critical for 4.2.
> Though I thought it's not harm, if needs I can revert it from 4.2 but
> still keep it in master.
>
> > -----Original Message-----
> > From: Animesh Chaturvedi
> > Sent: Monday, August 12, 2013 5:37 PM
> > To: dev@cloudstack.apache.org; Frank Zhang; John Burwell
> > Cc: Laszlo Hornyak
> > Subject: RE: Review Request 11942: ProcessUtil cleanup
> >
> > Is this really needed for 4.2? 4.2 is in limited updates and only
> > blocker/critical bug fixes and doc and test updates are allowed. All
> > commits to 4.2 should have associated BUG id.
> >
> > > -----Original Message-----
> > > From: Frank Zhang [mailto:noreply@reviews.apache.org] On Behalf Of
> > > Frank Zhang
> > > Sent: Monday, August 12, 2013 2:43 PM
> > > To: John Burwell; Frank Zhang
> > > Cc: Laszlo Hornyak; cloudstack
> > > Subject: Re: Review Request 11942: ProcessUtil cleanup
> > >
> > >
> > > -----------------------------------------------------------
> > > This is an automatically generated e-mail. To reply, visit:
> > > https://reviews.apache.org/r/11942/#review25036
> > > -----------------------------------------------------------
> > >
> > > Ship it!
> > >
> > >
> > > This is very straightforward patch. Java filesystem api is known as
> > > unnecessary complex, we should take advantage of Apache common-io as
> > > much as possible.
> > > shipped in 4.2 and master
> > >
> > > - Frank Zhang
> > >
> > >
> > > On July 24, 2013, 4:42 a.m., Laszlo Hornyak wrote:
> > > >
> > > > -----------------------------------------------------------
> > > > This is an automatically generated e-mail. To reply, visit:
> > > > https://reviews.apache.org/r/11942/
> > > > -----------------------------------------------------------
> > > >
> > > > (Updated July 24, 2013, 4:42 a.m.)
> > > >
> > > >
> > > > Review request for cloudstack, Frank Zhang and John Burwell.
> > > >
> > > >
> > > > Repository: cloudstack-git
> > > >
> > > >
> > > > Description
> > > > -------
> > > >
> > > > - possible resource leak closed
> > > > - file content read uses now commons-lang FileUtils
> > > > - Added unit tests
> > > >
> > > >
> > > > Diffs
> > > > -----
> > > >
> > > > utils/src/com/cloud/utils/ProcessUtil.java c9fdf35
> > > > utils/test/com/cloud/utils/ProcessUtilTest.java PRE-CREATION
> > > >
> > > > Diff: https://reviews.apache.org/r/11942/diff/
> > > >
> > > >
> > > > Testing
> > > > -------
> > > >
> > > > test included
> > > >
> > > >
> > > > Thanks,
> > > >
> > > > Laszlo Hornyak
> > > >
> > > >
RE: Review Request 11942: ProcessUtil cleanup
Posted by Frank Zhang <Fr...@citrix.com>.
Emm. Actually it's not a blocker or even critical for 4.2.
Though I thought it's not harm, if needs I can revert it from 4.2 but still keep it in master.
> -----Original Message-----
> From: Animesh Chaturvedi
> Sent: Monday, August 12, 2013 5:37 PM
> To: dev@cloudstack.apache.org; Frank Zhang; John Burwell
> Cc: Laszlo Hornyak
> Subject: RE: Review Request 11942: ProcessUtil cleanup
>
> Is this really needed for 4.2? 4.2 is in limited updates and only blocker/critical
> bug fixes and doc and test updates are allowed. All commits to 4.2 should have
> associated BUG id.
>
> > -----Original Message-----
> > From: Frank Zhang [mailto:noreply@reviews.apache.org] On Behalf Of
> > Frank Zhang
> > Sent: Monday, August 12, 2013 2:43 PM
> > To: John Burwell; Frank Zhang
> > Cc: Laszlo Hornyak; cloudstack
> > Subject: Re: Review Request 11942: ProcessUtil cleanup
> >
> >
> > -----------------------------------------------------------
> > This is an automatically generated e-mail. To reply, visit:
> > https://reviews.apache.org/r/11942/#review25036
> > -----------------------------------------------------------
> >
> > Ship it!
> >
> >
> > This is very straightforward patch. Java filesystem api is known as
> > unnecessary complex, we should take advantage of Apache common-io as
> > much as possible.
> > shipped in 4.2 and master
> >
> > - Frank Zhang
> >
> >
> > On July 24, 2013, 4:42 a.m., Laszlo Hornyak wrote:
> > >
> > > -----------------------------------------------------------
> > > This is an automatically generated e-mail. To reply, visit:
> > > https://reviews.apache.org/r/11942/
> > > -----------------------------------------------------------
> > >
> > > (Updated July 24, 2013, 4:42 a.m.)
> > >
> > >
> > > Review request for cloudstack, Frank Zhang and John Burwell.
> > >
> > >
> > > Repository: cloudstack-git
> > >
> > >
> > > Description
> > > -------
> > >
> > > - possible resource leak closed
> > > - file content read uses now commons-lang FileUtils
> > > - Added unit tests
> > >
> > >
> > > Diffs
> > > -----
> > >
> > > utils/src/com/cloud/utils/ProcessUtil.java c9fdf35
> > > utils/test/com/cloud/utils/ProcessUtilTest.java PRE-CREATION
> > >
> > > Diff: https://reviews.apache.org/r/11942/diff/
> > >
> > >
> > > Testing
> > > -------
> > >
> > > test included
> > >
> > >
> > > Thanks,
> > >
> > > Laszlo Hornyak
> > >
> > >
RE: Review Request 11942: ProcessUtil cleanup
Posted by Animesh Chaturvedi <an...@citrix.com>.
Is this really needed for 4.2? 4.2 is in limited updates and only blocker/critical bug fixes and doc and test updates are allowed. All commits to 4.2 should have associated BUG id.
> -----Original Message-----
> From: Frank Zhang [mailto:noreply@reviews.apache.org] On Behalf Of Frank
> Zhang
> Sent: Monday, August 12, 2013 2:43 PM
> To: John Burwell; Frank Zhang
> Cc: Laszlo Hornyak; cloudstack
> Subject: Re: Review Request 11942: ProcessUtil cleanup
>
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11942/#review25036
> -----------------------------------------------------------
>
> Ship it!
>
>
> This is very straightforward patch. Java filesystem api is known as
> unnecessary complex, we should take advantage of Apache common-io as
> much as possible.
> shipped in 4.2 and master
>
> - Frank Zhang
>
>
> On July 24, 2013, 4:42 a.m., Laszlo Hornyak wrote:
> >
> > -----------------------------------------------------------
> > This is an automatically generated e-mail. To reply, visit:
> > https://reviews.apache.org/r/11942/
> > -----------------------------------------------------------
> >
> > (Updated July 24, 2013, 4:42 a.m.)
> >
> >
> > Review request for cloudstack, Frank Zhang and John Burwell.
> >
> >
> > Repository: cloudstack-git
> >
> >
> > Description
> > -------
> >
> > - possible resource leak closed
> > - file content read uses now commons-lang FileUtils
> > - Added unit tests
> >
> >
> > Diffs
> > -----
> >
> > utils/src/com/cloud/utils/ProcessUtil.java c9fdf35
> > utils/test/com/cloud/utils/ProcessUtilTest.java PRE-CREATION
> >
> > Diff: https://reviews.apache.org/r/11942/diff/
> >
> >
> > Testing
> > -------
> >
> > test included
> >
> >
> > Thanks,
> >
> > Laszlo Hornyak
> >
> >
Re: Review Request 11942: ProcessUtil cleanup
Posted by Frank Zhang <fr...@citrix.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11942/#review25036
-----------------------------------------------------------
Ship it!
This is very straightforward patch. Java filesystem api is known as unnecessary complex, we should take advantage of Apache common-io as much as possible.
shipped in 4.2 and master
- Frank Zhang
On July 24, 2013, 4:42 a.m., Laszlo Hornyak wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11942/
> -----------------------------------------------------------
>
> (Updated July 24, 2013, 4:42 a.m.)
>
>
> Review request for cloudstack, Frank Zhang and John Burwell.
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> - possible resource leak closed
> - file content read uses now commons-lang FileUtils
> - Added unit tests
>
>
> Diffs
> -----
>
> utils/src/com/cloud/utils/ProcessUtil.java c9fdf35
> utils/test/com/cloud/utils/ProcessUtilTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/11942/diff/
>
>
> Testing
> -------
>
> test included
>
>
> Thanks,
>
> Laszlo Hornyak
>
>
Re: Review Request 11942: ProcessUtil cleanup
Posted by Laszlo Hornyak <la...@gmail.com>.
> On Aug. 14, 2013, 7:48 a.m., Wei Zhou wrote:
> > An issue related to this patch: https://issues.apache.org/jira/browse/CLOUDSTACK-4314
> > and patch: https://git-wip-us.apache.org/repos/asf?p=cloudstack.git;h=eb59c3c
> > Testing ok on devcloud.
indeed I did not test with the environment.properties on path. Sorry for the inconvenience and thank you for the fix!
- Laszlo
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11942/#review25127
-----------------------------------------------------------
On July 24, 2013, 4:42 a.m., Laszlo Hornyak wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11942/
> -----------------------------------------------------------
>
> (Updated July 24, 2013, 4:42 a.m.)
>
>
> Review request for cloudstack, Frank Zhang and John Burwell.
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> - possible resource leak closed
> - file content read uses now commons-lang FileUtils
> - Added unit tests
>
>
> Diffs
> -----
>
> utils/src/com/cloud/utils/ProcessUtil.java c9fdf35
> utils/test/com/cloud/utils/ProcessUtilTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/11942/diff/
>
>
> Testing
> -------
>
> test included
>
>
> Thanks,
>
> Laszlo Hornyak
>
>
Re: Review Request 11942: ProcessUtil cleanup
Posted by Wei Zhou <w....@leaseweb.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11942/#review25127
-----------------------------------------------------------
An issue related to this patch: https://issues.apache.org/jira/browse/CLOUDSTACK-4314
and patch: https://git-wip-us.apache.org/repos/asf?p=cloudstack.git;h=eb59c3c
Testing ok on devcloud.
- Wei Zhou
On July 24, 2013, 4:42 a.m., Laszlo Hornyak wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11942/
> -----------------------------------------------------------
>
> (Updated July 24, 2013, 4:42 a.m.)
>
>
> Review request for cloudstack, Frank Zhang and John Burwell.
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> - possible resource leak closed
> - file content read uses now commons-lang FileUtils
> - Added unit tests
>
>
> Diffs
> -----
>
> utils/src/com/cloud/utils/ProcessUtil.java c9fdf35
> utils/test/com/cloud/utils/ProcessUtilTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/11942/diff/
>
>
> Testing
> -------
>
> test included
>
>
> Thanks,
>
> Laszlo Hornyak
>
>
Re: Review Request 11942: ProcessUtil cleanup
Posted by Prasanna Santhanam <ts...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11942/
-----------------------------------------------------------
(Updated July 24, 2013, 4:42 a.m.)
Review request for cloudstack, Frank Zhang and John Burwell.
Changes
-------
adding couple reviewers
Repository: cloudstack-git
Description
-------
- possible resource leak closed
- file content read uses now commons-lang FileUtils
- Added unit tests
Diffs
-----
utils/src/com/cloud/utils/ProcessUtil.java c9fdf35
utils/test/com/cloud/utils/ProcessUtilTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/11942/diff/
Testing
-------
test included
Thanks,
Laszlo Hornyak
Re: Review Request 11942: ProcessUtil cleanup
Posted by Laszlo Hornyak <la...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11942/
-----------------------------------------------------------
(Updated July 20, 2013, 8:01 p.m.)
Review request for cloudstack.
Changes
-------
no change, only rebased
Repository: cloudstack-git
Description
-------
- possible resource leak closed
- file content read uses now commons-lang FileUtils
- Added unit tests
Diffs (updated)
-----
utils/src/com/cloud/utils/ProcessUtil.java c9fdf35
utils/test/com/cloud/utils/ProcessUtilTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/11942/diff/
Testing
-------
test included
Thanks,
Laszlo Hornyak
Re: Review Request 11942: ProcessUtil cleanup
Posted by Laszlo Hornyak <la...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11942/
-----------------------------------------------------------
(Updated July 4, 2013, 9:34 p.m.)
Review request for cloudstack.
Changes
-------
added assertions to positive test
Repository: cloudstack-git
Description
-------
- possible resource leak closed
- file content read uses now commons-lang FileUtils
- Added unit tests
Diffs (updated)
-----
utils/src/com/cloud/utils/ProcessUtil.java c9fdf35
utils/test/com/cloud/utils/ProcessUtilTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/11942/diff/
Testing
-------
test included
Thanks,
Laszlo Hornyak
Re: Review Request 11942: ProcessUtil cleanup
Posted by Laszlo Hornyak <la...@gmail.com>.
> On July 1, 2013, 3:04 p.m., John Burwell wrote:
> > utils/test/com/cloud/utils/ProcessUtilTest.java, line 34
> > <https://reviews.apache.org/r/11942/diff/2/?file=313280#file313280line34>
> >
> > Is there no system state on which to assert? What would cause this test to fail?
This test verifies that the method with such arguments fails, that's why it expects an exception.
> On July 1, 2013, 3:04 p.m., John Burwell wrote:
> > utils/test/com/cloud/utils/ProcessUtilTest.java, line 40
> > <https://reviews.apache.org/r/11942/diff/2/?file=313280#file313280line40>
> >
> > Is there no system state on which to assert? What would cause this test to fail?
This test verifies that the method with such arguments fails, that's why it expects an exception.
> On July 1, 2013, 3:04 p.m., John Burwell wrote:
> > utils/test/com/cloud/utils/ProcessUtilTest.java, line 28
> > <https://reviews.apache.org/r/11942/diff/2/?file=313280#file313280line28>
> >
> > Is there no system state on which to assert? What would cause this test to fail?
Actually here I can add an assertion that the pid of this java process is written to the pid file, but I won't really be able to verify the pid in a platform independent way, since Java does not give an API to find out the PID of the current process. I can only use the very same method that the tested code is using, therefore an assume(is_os_linux) is also needed here.
The original purpose was to remove the minor resource leak from the original code (line 71) but I did not want to add a test that calls the method a lots of times.
- Laszlo
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11942/#review22599
-----------------------------------------------------------
On June 29, 2013, 3:51 p.m., Laszlo Hornyak wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11942/
> -----------------------------------------------------------
>
> (Updated June 29, 2013, 3:51 p.m.)
>
>
> Review request for cloudstack.
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> - possible resource leak closed
> - file content read uses now commons-lang FileUtils
> - Added unit tests
>
>
> Diffs
> -----
>
> utils/src/com/cloud/utils/ProcessUtil.java c9fdf35
> utils/test/com/cloud/utils/ProcessUtilTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/11942/diff/
>
>
> Testing
> -------
>
> test included
>
>
> Thanks,
>
> Laszlo Hornyak
>
>
Re: Review Request 11942: ProcessUtil cleanup
Posted by John Burwell <jb...@basho.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11942/#review22599
-----------------------------------------------------------
utils/test/com/cloud/utils/ProcessUtilTest.java
<https://reviews.apache.org/r/11942/#comment46302>
Is there no system state on which to assert? What would cause this test to fail?
utils/test/com/cloud/utils/ProcessUtilTest.java
<https://reviews.apache.org/r/11942/#comment46303>
Is there no system state on which to assert? What would cause this test to fail?
utils/test/com/cloud/utils/ProcessUtilTest.java
<https://reviews.apache.org/r/11942/#comment46304>
Is there no system state on which to assert? What would cause this test to fail?
- John Burwell
On June 29, 2013, 3:51 p.m., Laszlo Hornyak wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11942/
> -----------------------------------------------------------
>
> (Updated June 29, 2013, 3:51 p.m.)
>
>
> Review request for cloudstack.
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> - possible resource leak closed
> - file content read uses now commons-lang FileUtils
> - Added unit tests
>
>
> Diffs
> -----
>
> utils/src/com/cloud/utils/ProcessUtil.java c9fdf35
> utils/test/com/cloud/utils/ProcessUtilTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/11942/diff/
>
>
> Testing
> -------
>
> test included
>
>
> Thanks,
>
> Laszlo Hornyak
>
>
Re: Review Request 11942: ProcessUtil cleanup
Posted by Laszlo Hornyak <la...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11942/
-----------------------------------------------------------
(Updated June 29, 2013, 3:51 p.m.)
Review request for cloudstack.
Changes
-------
basically just rebased
Repository: cloudstack-git
Description
-------
- possible resource leak closed
- file content read uses now commons-lang FileUtils
- Added unit tests
Diffs (updated)
-----
utils/src/com/cloud/utils/ProcessUtil.java c9fdf35
utils/test/com/cloud/utils/ProcessUtilTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/11942/diff/
Testing
-------
test included
Thanks,
Laszlo Hornyak