> 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 > >
