Re: Review Request 60626: Eliminated os::shell calls from HDFS for Windows compatibility.

[Andrew Schwartzmeyer]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60626/#review193295
---


Ship it!




Ship It!

- Andrew Schwartzmeyer


On Dec. 7, 2017, 6:44 p.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60626/
> ---
> 
> (Updated Dec. 7, 2017, 6:44 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, Li 
> Li, and Michael Park.
> 
> 
> 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/7/
> 
> 
> Testing
> ---
> 
> See upstream
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 60626: Eliminated os::shell calls from HDFS for Windows compatibility.

[Jeff Coffler]

> On Nov. 16, 2017, 11:34 p.m., Jie Yu wrote:
> > src/hdfs/hdfs.cpp
> > Line 119 (original), 119 (patched)
> > 
> >
> > i'd kill this line to be consistent with others.

I don't think you're looking at the latest version, I don't have this line.


- Jeff


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60626/#review191269
---


On Nov. 6, 2017, 6:09 p.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60626/
> ---
> 
> (Updated Nov. 6, 2017, 6:09 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, Li 
> Li, and Michael Park.
> 
> 
> 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/6/
> 
> 
> Testing
> ---
> 
> See upstream
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 60626: Eliminated os::shell calls from HDFS for Windows compatibility.

[Jeff Coffler]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60626/
---

(Updated Dec. 8, 2017, 2:44 a.m.)


Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, Li Li, 
and Michael Park.


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

Changes: https://reviews.apache.org/r/60626/diff/6-7/


Testing
---

See upstream


Thanks,

Jeff Coffler

Re: Review Request 60626: Eliminated os::shell calls from HDFS for Windows compatibility.

[Jie Yu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60626/#review191269
---


Fix it, then Ship it!





src/hdfs/hdfs.cpp
Line 119 (original), 119 (patched)


i'd kill this line to be consistent with others.



src/hdfs/hdfs.cpp
Lines 134 (patched)


Please put `+` to the end of the line above


- Jie Yu


On Nov. 6, 2017, 6:09 p.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60626/
> ---
> 
> (Updated Nov. 6, 2017, 6:09 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, Li 
> Li, and Michael Park.
> 
> 
> 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/6/
> 
> 
> Testing
> ---
> 
> See upstream
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 60626: Eliminated os::shell calls from HDFS for Windows compatibility.

[Jeff Coffler]

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

[Andrew Schwartzmeyer]

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

[Jeff Coffler]

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

[Jeff Coffler]

> 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)
> > 
> >
> > nit: Maybe it's just me, but this could just be `Option 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.

[Andrew Schwartzmeyer]

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


nit: Maybe it's just me, but this could just be `Option 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.

[Jeff Coffler]

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

[Jeff Coffler]

> On Oct. 8, 2017, 4:16 a.m., Andrew Schwartzmeyer wrote:
> > src/hdfs/hdfs.cpp
> > Lines 117-118 (original), 117-118 (patched)
> > 
> >
> > 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)
> > 
> >
> > 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)
> > 
> >
> > 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)
> > 
> >
> > `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)
> > 
> >
> > `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/
> ---

Re: Review Request 60626: Eliminated os::shell calls from HDFS for Windows compatibility.

[Jeff Coffler]

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