You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jeff Coffler <je...@taltos.com> on 2017/07/03 19:34:28 UTC
Review Request 60626: Eliminate os::shell calls from HDFS for Windows
compatibility.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60626/
-----------------------------------------------------------
Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and Li Li.
Bugs: MESOS-6705
https://issues.apache.org/jira/browse/MESOS-6705
Repository: mesos
Description
-------
Eliminate os::shell calls from HDFS for Windows compatibility.
Diffs
-----
src/hdfs/hdfs.cpp 2c95a5ea43a4289e1168c527b9ccc35690a751a4
Diff: https://reviews.apache.org/r/60626/diff/1/
Testing
-------
See upstream
Thanks,
Jeff Coffler
Re: Review Request 60626: Eliminated os::shell calls from HDFS for
Windows compatibility.
Posted by Jeff Coffler <je...@taltos.com>.
> On Oct. 8, 2017, 4:16 a.m., Andrew Schwartzmeyer wrote:
> > src/hdfs/hdfs.cpp
> > Lines 117-118 (original), 117-118 (patched)
> > <https://reviews.apache.org/r/60626/diff/2/?file=1847164#file1847164line117>
> >
> > I believe that [this overload](https://github.com/apache/mesos/blob/5e5ed4db273179101eee90d752c1622315987949/3rdparty/libprocess/include/process/subprocess.hpp#L410) implies the subprocess's stdout and stderr FDs are not directed toward the parent's, and so the `2>&1` is unnecessary. Test it to be sure, but that's what I'm reading.
I don't see this. It looks to me like the caller wanted any errors to be treated as STDOUT, and the link you showed clearly shows that STDOUT and STDERR are different. No, they're not directed to the parent, as such, but they are directed to different places. And the command clearly didn't want that.
> On Oct. 8, 2017, 4:16 a.m., Andrew Schwartzmeyer wrote:
> > src/hdfs/hdfs.cpp
> > Lines 125-128 (patched)
> > <https://reviews.apache.org/r/60626/diff/2/?file=1847164#file1847164line125>
> >
> > Why `o_status`, why not just `status`?
This wasn't the final status, it was the subprocess status (in case subprocess couldn't be launched). Renamed to subStatus.
> On Oct. 8, 2017, 4:16 a.m., Andrew Schwartzmeyer wrote:
> > src/hdfs/hdfs.cpp
> > Lines 126 (patched)
> > <https://reviews.apache.org/r/60626/diff/2/?file=1847164#file1847164line126>
> >
> > We can _probably_ skip this particular check, in the specicial case of the `status()` of a `process::subprocess`, because of [this (supposed) guarantee](https://github.com/apache/mesos/blob/5e5ed4db273179101eee90d752c1622315987949/3rdparty/libprocess/include/process/subprocess.hpp#L292).
I don't think this is true. This check is in case subprocess itself fails to launch. Had this been true, then calling get().get() would have been safe. But it wasn't in the case of subprocess launch failure.
The guarantee you refer to is based on if subprocess has launched the process, I believe. This is not that. Please clarify.
> On Oct. 8, 2017, 4:16 a.m., Andrew Schwartzmeyer wrote:
> > src/hdfs/hdfs.cpp
> > Lines 130-134 (patched)
> > <https://reviews.apache.org/r/60626/diff/2/?file=1847164#file1847164line130>
> >
> > `s/f_status/status.get()/g`.
> >
> > We probably don't need to stringify and return the exit status.
>
> Jeff Coffler wrote:
> I actually got the error during testing (due to an unrelated problem), and an error like "hey, it failed, I'm not telling you anything more" was useless for me. The more information the better in case of errors, and I think the exit status is reasonable to give in this case.
>
> Assuming that the hadoop client gives meaningful error codes on return, this is even more important.
>
> I (already) changed the f_status to status, though, when I got rid of o_status.
I actually got the error during testing (due to an unrelated problem), and an error like "hey, it failed, I'm not telling you anything more" was useless for me. The more information the better in case of errors, and I think the exit status is reasonable to give in this case.
Assuming that the hadoop client gives meaningful error codes on return, this is even more important.
> On Oct. 8, 2017, 4:16 a.m., Andrew Schwartzmeyer wrote:
> > src/hdfs/hdfs.cpp
> > Lines 130-134 (patched)
> > <https://reviews.apache.org/r/60626/diff/2/?file=1847164#file1847164line130>
> >
> > `s/f_status/status.get()/g`.
> >
> > We probably don't need to stringify and return the exit status.
>
> Jeff Coffler wrote:
> I actually got the error during testing (due to an unrelated problem), and an error like "hey, it failed, I'm not telling you anything more" was useless for me. The more information the better in case of errors, and I think the exit status is reasonable to give in this case.
>
> Assuming that the hadoop client gives meaningful error codes on return, this is even more important.
I actually got the error during testing (due to an unrelated problem), and an error like "hey, it failed, I'm not telling you anything more" was useless for me. The more information the better in case of errors, and I think the exit status is reasonable to give in this case.
Assuming that the hadoop client gives meaningful error codes on return, this is even more important.
I (already) changed the f_status to status, though, when I got rid of o_status.
- Jeff
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60626/#review187356
-----------------------------------------------------------
On Oct. 11, 2017, 11:31 p.m., Jeff Coffler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60626/
> -----------------------------------------------------------
>
> (Updated Oct. 11, 2017, 11:31 p.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and Li Li.
>
>
> Bugs: MESOS-6705
> https://issues.apache.org/jira/browse/MESOS-6705
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Eliminated os::shell calls from HDFS for Windows compatibility.
>
>
> Diffs
> -----
>
> src/hdfs/hdfs.cpp 2c95a5ea43a4289e1168c527b9ccc35690a751a4
>
>
> Diff: https://reviews.apache.org/r/60626/diff/3/
>
>
> Testing
> -------
>
> See upstream
>
>
> Thanks,
>
> Jeff Coffler
>
>
Re: Review Request 60626: Eliminate os::shell calls from HDFS for
Windows compatibility.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60626/#review187356
-----------------------------------------------------------
Summary and description need to follow Mesos standards (past-tense, full-sentence summary, with description in the body).
src/hdfs/hdfs.cpp
Lines 117-118 (original), 117-118 (patched)
<https://reviews.apache.org/r/60626/#comment264316>
I believe that [this overload](https://github.com/apache/mesos/blob/5e5ed4db273179101eee90d752c1622315987949/3rdparty/libprocess/include/process/subprocess.hpp#L410) implies the subprocess's stdout and stderr FDs are not directed toward the parent's, and so the `2>&1` is unnecessary. Test it to be sure, but that's what I'm reading.
src/hdfs/hdfs.cpp
Lines 118-120 (original), 118-123 (patched)
<https://reviews.apache.org/r/60626/#comment264312>
We try to avoid single character variable names (exceptions being like `i` or `x` in iterations).
This should proably be `subprocess`, or `hadoop_version`, or something.
src/hdfs/hdfs.cpp
Lines 125-128 (patched)
<https://reviews.apache.org/r/60626/#comment264313>
Why `o_status`, why not just `status`?
src/hdfs/hdfs.cpp
Lines 126 (patched)
<https://reviews.apache.org/r/60626/#comment264315>
We can _probably_ skip this particular check, in the specicial case of the `status()` of a `process::subprocess`, because of [this (supposed) guarantee](https://github.com/apache/mesos/blob/5e5ed4db273179101eee90d752c1622315987949/3rdparty/libprocess/include/process/subprocess.hpp#L292).
src/hdfs/hdfs.cpp
Lines 130-134 (patched)
<https://reviews.apache.org/r/60626/#comment264314>
`s/f_status/status.get()/g`.
We probably don't need to stringify and return the exit status.
- Andrew Schwartzmeyer
On Oct. 5, 2017, 9:04 p.m., Jeff Coffler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60626/
> -----------------------------------------------------------
>
> (Updated Oct. 5, 2017, 9:04 p.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and Li Li.
>
>
> Bugs: MESOS-6705
> https://issues.apache.org/jira/browse/MESOS-6705
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Eliminate os::shell calls from HDFS for Windows compatibility.
>
>
> Diffs
> -----
>
> src/hdfs/hdfs.cpp 2c95a5ea43a4289e1168c527b9ccc35690a751a4
>
>
> Diff: https://reviews.apache.org/r/60626/diff/2/
>
>
> Testing
> -------
>
> See upstream
>
>
> Thanks,
>
> Jeff Coffler
>
>
Re: Review Request 60626: Eliminated os::shell calls from HDFS for
Windows compatibility.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60626/#review188763
-----------------------------------------------------------
Ship it!
Ship It!
- Andrew Schwartzmeyer
On Oct. 19, 2017, 11:17 a.m., Jeff Coffler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60626/
> -----------------------------------------------------------
>
> (Updated Oct. 19, 2017, 11:17 a.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and Li Li.
>
>
> Bugs: MESOS-6705
> https://issues.apache.org/jira/browse/MESOS-6705
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Windows doesn't support os::shell calls, so reworked the code to use
> libprocess instead.
>
>
> Diffs
> -----
>
> src/hdfs/hdfs.cpp 2c95a5ea43a4289e1168c527b9ccc35690a751a4
>
>
> Diff: https://reviews.apache.org/r/60626/diff/5/
>
>
> Testing
> -------
>
> See upstream
>
>
> Thanks,
>
> Jeff Coffler
>
>
Re: Review Request 60626: Eliminated os::shell calls from HDFS for
Windows compatibility.
Posted by Jeff Coffler <je...@taltos.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60626/
-----------------------------------------------------------
(Updated Oct. 19, 2017, 9:33 p.m.)
Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and Li Li.
Bugs: MESOS-6705
https://issues.apache.org/jira/browse/MESOS-6705
Repository: mesos
Description
-------
Windows doesn't support os::shell calls, so reworked the code to use
libprocess instead.
Diffs (updated)
-----
src/hdfs/hdfs.cpp 2c95a5ea43a4289e1168c527b9ccc35690a751a4
Diff: https://reviews.apache.org/r/60626/diff/6/
Changes: https://reviews.apache.org/r/60626/diff/5-6/
Testing
-------
See upstream
Thanks,
Jeff Coffler
Re: Review Request 60626: Eliminated os::shell calls from HDFS for
Windows compatibility.
Posted by Jeff Coffler <je...@taltos.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60626/
-----------------------------------------------------------
(Updated Oct. 19, 2017, 6:17 p.m.)
Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and Li Li.
Bugs: MESOS-6705
https://issues.apache.org/jira/browse/MESOS-6705
Repository: mesos
Description (updated)
-------
Windows doesn't support os::shell calls, so reworked the code to use
libprocess instead.
Diffs (updated)
-----
src/hdfs/hdfs.cpp 2c95a5ea43a4289e1168c527b9ccc35690a751a4
Diff: https://reviews.apache.org/r/60626/diff/5/
Changes: https://reviews.apache.org/r/60626/diff/4-5/
Testing
-------
See upstream
Thanks,
Jeff Coffler
Re: Review Request 60626: Eliminated os::shell calls from HDFS for
Windows compatibility.
Posted by Jeff Coffler <je...@taltos.com>.
> On Oct. 18, 2017, 12:45 a.m., Andrew Schwartzmeyer wrote:
> > Needs a description still.
Ok.
> On Oct. 18, 2017, 12:45 a.m., Andrew Schwartzmeyer wrote:
> > src/hdfs/hdfs.cpp
> > Lines 125-134 (patched)
> > <https://reviews.apache.org/r/60626/diff/4/?file=1858873#file1858873line125>
> >
> > nit: Maybe it's just me, but this could just be `Option<int> status` and then `status.get()` below; that's _usually_ how it's done in the codebase.
Could have. But I don't like the idea of resolving get() multiple times. Six of one, dozen of another. I'll leave this one to Joe to change if he wants.
- Jeff
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60626/#review188428
-----------------------------------------------------------
On Oct. 17, 2017, 1:19 a.m., Jeff Coffler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60626/
> -----------------------------------------------------------
>
> (Updated Oct. 17, 2017, 1:19 a.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and Li Li.
>
>
> Bugs: MESOS-6705
> https://issues.apache.org/jira/browse/MESOS-6705
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Eliminated os::shell calls from HDFS for Windows compatibility.
>
>
> Diffs
> -----
>
> src/hdfs/hdfs.cpp 2c95a5ea43a4289e1168c527b9ccc35690a751a4
>
>
> Diff: https://reviews.apache.org/r/60626/diff/4/
>
>
> Testing
> -------
>
> See upstream
>
>
> Thanks,
>
> Jeff Coffler
>
>
Re: Review Request 60626: Eliminated os::shell calls from HDFS for
Windows compatibility.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60626/#review188428
-----------------------------------------------------------
Needs a description still.
src/hdfs/hdfs.cpp
Lines 125-134 (patched)
<https://reviews.apache.org/r/60626/#comment265423>
nit: Maybe it's just me, but this could just be `Option<int> status` and then `status.get()` below; that's _usually_ how it's done in the codebase.
- Andrew Schwartzmeyer
On Oct. 16, 2017, 6:19 p.m., Jeff Coffler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60626/
> -----------------------------------------------------------
>
> (Updated Oct. 16, 2017, 6:19 p.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and Li Li.
>
>
> Bugs: MESOS-6705
> https://issues.apache.org/jira/browse/MESOS-6705
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Eliminated os::shell calls from HDFS for Windows compatibility.
>
>
> Diffs
> -----
>
> src/hdfs/hdfs.cpp 2c95a5ea43a4289e1168c527b9ccc35690a751a4
>
>
> Diff: https://reviews.apache.org/r/60626/diff/4/
>
>
> Testing
> -------
>
> See upstream
>
>
> Thanks,
>
> Jeff Coffler
>
>
Re: Review Request 60626: Eliminated os::shell calls from HDFS for
Windows compatibility.
Posted by Jeff Coffler <je...@taltos.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60626/
-----------------------------------------------------------
(Updated Oct. 17, 2017, 1:19 a.m.)
Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and Li Li.
Bugs: MESOS-6705
https://issues.apache.org/jira/browse/MESOS-6705
Repository: mesos
Description
-------
Eliminated os::shell calls from HDFS for Windows compatibility.
Diffs (updated)
-----
src/hdfs/hdfs.cpp 2c95a5ea43a4289e1168c527b9ccc35690a751a4
Diff: https://reviews.apache.org/r/60626/diff/4/
Changes: https://reviews.apache.org/r/60626/diff/3-4/
Testing
-------
See upstream
Thanks,
Jeff Coffler
Re: Review Request 60626: Eliminated os::shell calls from HDFS for
Windows compatibility.
Posted by Jeff Coffler <je...@taltos.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60626/
-----------------------------------------------------------
(Updated Oct. 11, 2017, 11:31 p.m.)
Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and Li Li.
Summary (updated)
-----------------
Eliminated os::shell calls from HDFS for Windows compatibility.
Bugs: MESOS-6705
https://issues.apache.org/jira/browse/MESOS-6705
Repository: mesos
Description (updated)
-------
Eliminated os::shell calls from HDFS for Windows compatibility.
Diffs (updated)
-----
src/hdfs/hdfs.cpp 2c95a5ea43a4289e1168c527b9ccc35690a751a4
Diff: https://reviews.apache.org/r/60626/diff/3/
Changes: https://reviews.apache.org/r/60626/diff/2-3/
Testing
-------
See upstream
Thanks,
Jeff Coffler
Re: Review Request 60626: Eliminate os::shell calls from HDFS for
Windows compatibility.
Posted by Jeff Coffler <je...@taltos.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60626/
-----------------------------------------------------------
(Updated Oct. 6, 2017, 4:04 a.m.)
Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and Li Li.
Bugs: MESOS-6705
https://issues.apache.org/jira/browse/MESOS-6705
Repository: mesos
Description
-------
Eliminate os::shell calls from HDFS for Windows compatibility.
Diffs (updated)
-----
src/hdfs/hdfs.cpp 2c95a5ea43a4289e1168c527b9ccc35690a751a4
Diff: https://reviews.apache.org/r/60626/diff/2/
Changes: https://reviews.apache.org/r/60626/diff/1-2/
Testing
-------
See upstream
Thanks,
Jeff Coffler
Re: Review Request 60626: Eliminate os::shell calls from HDFS for
Windows compatibility.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60626/#review182977
-----------------------------------------------------------
src/hdfs/hdfs.cpp
Lines 118-120 (original), 118-121 (patched)
<https://reviews.apache.org/r/60626/#comment258952>
This needs to have further error handling. You can't just `get().get()`, as it may error.
Also, this subtly changed the style; there's a floating open curly brace.
- Andrew Schwartzmeyer
On July 3, 2017, 12:34 p.m., Jeff Coffler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60626/
> -----------------------------------------------------------
>
> (Updated July 3, 2017, 12:34 p.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and Li Li.
>
>
> Bugs: MESOS-6705
> https://issues.apache.org/jira/browse/MESOS-6705
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Eliminate os::shell calls from HDFS for Windows compatibility.
>
>
> Diffs
> -----
>
> src/hdfs/hdfs.cpp 2c95a5ea43a4289e1168c527b9ccc35690a751a4
>
>
> Diff: https://reviews.apache.org/r/60626/diff/1/
>
>
> Testing
> -------
>
> See upstream
>
>
> Thanks,
>
> Jeff Coffler
>
>