Re: Review Request 60624: Enable HDFS compilation and associated tests.

[Andrew Schwartzmeyer]

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



Summary and description need to follow Mesos standards (past-tense, 
full-sentence summary, with description in the body).

- 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/60624/
> ---
> 
> (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
> ---
> 
> Note that tests are disabled for Windows due to dependence on 'sh'
> shell.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 1a828c4351ded36f51ccbbe67147da2f50b9cdb1 
>   src/launcher/fetcher.cpp 5a9e93adbf2e4f0b8ff64a69e62fea5f6a0b8f2d 
>   src/tests/CMakeLists.txt def233d13f627f58362fdc61ac3e9c104d65fe7e 
>   src/tests/hdfs_tests.cpp e7154c75e663d9a98bec48be42a59b65c96f9515 
> 
> 
> Diff: https://reviews.apache.org/r/60624/diff/2/
> 
> 
> Testing
> ---
> 
> See upstream
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 60624: Enable HDFS compilation and associated tests.

[Andrew Schwartzmeyer]

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



What is the reason that the HDFS files are now made to build on Windows, but 
(AFAICT) not actually enabled for support? Is there an immediate plan to 
support HDFS, and if not, why start building the files?


src/launcher/fetcher.cpp
Lines 267-268 (patched)


If so, this needs a TODO and an associated issue. The form should be:

// TODO(coffler): Fix Windows chmod handling, see MESOS-1234.

Or an explanation of why we can just ignore this on Windows (exe bit 
doesn't matter!).



src/launcher/fetcher.cpp
Lines 485-487 (patched)


Ditto style comment above.



src/launcher/fetcher.cpp
Lines 564-569 (original), 571-579 (patched)


Ditto style comment above.

For ownership, we probably need a separate issue tracking its 
implementation.



src/tests/hdfs_tests.cpp
Lines 55-57 (original), 55-60 (patched)


Ditto above on style / also exe bit doesn't map on Windows, so we can 
saefly ignore this logic.



src/tests/hdfs_tests.cpp
Line 67 (original), 70 (patched)


All these disabled tests need an issue and note why they're disabled, as 
this patch is bringing the file in.


- 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/60624/
> ---
> 
> (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
> ---
> 
> Note that tests are disabled for Windows due to dependence on 'sh'
> shell.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 1a828c4351ded36f51ccbbe67147da2f50b9cdb1 
>   src/launcher/fetcher.cpp 5a9e93adbf2e4f0b8ff64a69e62fea5f6a0b8f2d 
>   src/tests/CMakeLists.txt def233d13f627f58362fdc61ac3e9c104d65fe7e 
>   src/tests/hdfs_tests.cpp e7154c75e663d9a98bec48be42a59b65c96f9515 
> 
> 
> Diff: https://reviews.apache.org/r/60624/diff/2/
> 
> 
> Testing
> ---
> 
> See upstream
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 60624: Enable HDFS compilation and associated tests.

[Jeff Coffler]

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

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

Note that tests are disabled for Windows due to dependence on 'sh'
shell.


Diffs (updated)
-

  src/CMakeLists.txt 1a828c4351ded36f51ccbbe67147da2f50b9cdb1 
  src/launcher/fetcher.cpp 5a9e93adbf2e4f0b8ff64a69e62fea5f6a0b8f2d 
  src/tests/CMakeLists.txt def233d13f627f58362fdc61ac3e9c104d65fe7e 
  src/tests/hdfs_tests.cpp e7154c75e663d9a98bec48be42a59b65c96f9515 


Diff: https://reviews.apache.org/r/60624/diff/2/

Changes: https://reviews.apache.org/r/60624/diff/1-2/


Testing
---

See upstream


Thanks,

Jeff Coffler

Re: Review Request 60624: Enable HDFS compilation and associated tests.

[Jeff Coffler]

> On Aug. 15, 2017, 6:11 p.m., Andrew Schwartzmeyer wrote:
> > src/launcher/fetcher.cpp
> > Lines 267 (patched)
> > 
> >
> > This one is straight-forward. On Windows, there is no "executable 
> > permissions" so we can simply ignore this logic. With a note of course as 
> > to why.

This needs to be generally revisited, and is on the list. This is a much bigger 
change, I want to handle it all at once.


> On Aug. 15, 2017, 6:11 p.m., Andrew Schwartzmeyer wrote:
> > src/launcher/fetcher.cpp
> > Lines 487 (patched)
> > 
> >
> > I'm not suggesting to do this _right now_. But for `chown`, Windows has 
> > a mappable concept of taking (probably recursive) ownership of the 
> > directory as the specified user. So we can handle this, we'll just need to 
> > implement it.

Permissions is an issue throughout Mesos today, and should be looked at at that 
time.


> On Aug. 15, 2017, 6:11 p.m., Andrew Schwartzmeyer wrote:
> > src/launcher/fetcher.cpp
> > Lines 564 (patched)
> > 
> >
> > I think we can do the same thing as `os::su` with Windows user 
> > impersonation. It's a mappable concept.

It's only mappable when we get the permissions thing worked out, and we can run 
on something other than an admistrative account.


> On Aug. 15, 2017, 6:11 p.m., Andrew Schwartzmeyer wrote:
> > src/tests/hdfs_tests.cpp
> > Lines 56 (patched)
> > 
> >
> > Like above, we can safely ignore setting execution permission on 
> > Windows. My reasoning is that the concept maps to nothing. So doing nothing 
> > is appropriate I think.

Again, this sort of thing is in many places (including other modules), and I'd 
like to fix that issue all at once.


- Jeff


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


On July 3, 2017, 7:32 p.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60624/
> ---
> 
> (Updated July 3, 2017, 7:32 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
> ---
> 
> Note that tests are disabled for Windows due to dependence on 'sh'
> shell.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 51b67428f823454665db695ba70a0dc896a7595c 
>   src/launcher/fetcher.cpp 42980f5a4a40b72f754156469e9fe60a952d1d87 
>   src/tests/CMakeLists.txt 9c0acaf43f451dbc9ba5077529a36aa4cef40c34 
>   src/tests/hdfs_tests.cpp e7154c75e663d9a98bec48be42a59b65c96f9515 
> 
> 
> Diff: https://reviews.apache.org/r/60624/diff/1/
> 
> 
> Testing
> ---
> 
> See upstream
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 60624: Enable HDFS compilation and associated tests.

[Andrew Schwartzmeyer]

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




src/CMakeLists.txt
Line 513 (original), 514 (patched)


Uh.



src/launcher/fetcher.cpp
Lines 267 (patched)


This one is straight-forward. On Windows, there is no "executable 
permissions" so we can simply ignore this logic. With a note of course as to 
why.



src/launcher/fetcher.cpp
Lines 487 (patched)


I'm not suggesting to do this _right now_. But for `chown`, Windows has a 
mappable concept of taking (probably recursive) ownership of the directory as 
the specified user. So we can handle this, we'll just need to implement it.



src/launcher/fetcher.cpp
Lines 564 (patched)


I think we can do the same thing as `os::su` with Windows user 
impersonation. It's a mappable concept.



src/tests/hdfs_tests.cpp
Lines 56 (patched)


Like above, we can safely ignore setting execution permission on Windows. 
My reasoning is that the concept maps to nothing. So doing nothing is 
appropriate I think.


- Andrew Schwartzmeyer


On July 3, 2017, 12:32 p.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60624/
> ---
> 
> (Updated July 3, 2017, 12:32 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
> ---
> 
> Note that tests are disabled for Windows due to dependence on 'sh'
> shell.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 51b67428f823454665db695ba70a0dc896a7595c 
>   src/launcher/fetcher.cpp 42980f5a4a40b72f754156469e9fe60a952d1d87 
>   src/tests/CMakeLists.txt 9c0acaf43f451dbc9ba5077529a36aa4cef40c34 
>   src/tests/hdfs_tests.cpp e7154c75e663d9a98bec48be42a59b65c96f9515 
> 
> 
> Diff: https://reviews.apache.org/r/60624/diff/1/
> 
> 
> Testing
> ---
> 
> See upstream
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Review Request 60624: Enable HDFS compilation and associated tests.

[Jeff Coffler]

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

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

Note that tests are disabled for Windows due to dependence on 'sh'
shell.


Diffs
-

  src/CMakeLists.txt 51b67428f823454665db695ba70a0dc896a7595c 
  src/launcher/fetcher.cpp 42980f5a4a40b72f754156469e9fe60a952d1d87 
  src/tests/CMakeLists.txt 9c0acaf43f451dbc9ba5077529a36aa4cef40c34 
  src/tests/hdfs_tests.cpp e7154c75e663d9a98bec48be42a59b65c96f9515 


Diff: https://reviews.apache.org/r/60624/diff/1/


Testing
---

See upstream


Thanks,

Jeff Coffler