> On Oct. 18, 2017, 1:36 a.m., Andrew Schwartzmeyer wrote: > > src/launcher/fetcher.cpp > > Lines 192-211 (original), 198-209 (patched) > > <https://reviews.apache.org/r/60628/diff/4/?file=1858875#file1858875line198> > > > > I've mentioned this a few times, but this function at least appears > > super redundant. > > > > It's only used in the fetcher's `download()` and `fetchFromCache()`. > > It's true that these are dumb and expect `Try<string> copy(a, b)` where the > > result is always `b` (or the error), and I just wish we could fix the use > > of it instead. > > > > Maybe at least leave a `// TODO: Refactor code to eliminate redundnant > > function.` Since it looks so funny to have `copyFile` return `copyfile`.
The problem is that this is NOT a redundant function. This is called in several places where other methods in those places also return a Try<string>, and stout's copyfile function doesn't return that (it did initially, but that was factored out in prior changes). I don't see the need to refactor other, unrelated code, just to get rid of this Try<string> handler. Note, by the way, that this function also implements logging that existed in the original fetcher code but does not exist in stout's copyfile function either, so it's not really redundant (in case the logging doesn't matter and, in general, I tend to think (within reason): the more logging the better). In any case, I've added a TODO as per your request. > On Oct. 18, 2017, 1:36 a.m., Andrew Schwartzmeyer wrote: > > src/launcher/fetcher.cpp > > Lines 267-272 (original), 265-273 (patched) > > <https://reviews.apache.org/r/60628/diff/4/?file=1858875#file1858875line273> > > > > I've mentioned in other reviews, but we _do know_ how to handle `chmod > > +x` for Windows: we explicitly don't do anything. I don't think this is a > > `TODO` but a `NOTE: Windows does not have the concept of executable > > permissions.` It's handled instead by file extensions (and a list in the > > registrary that says which extensions are "executable"). > > > > Correct me if I'm wrong, but I don't think we'll _ever_ do anything > > other than not execute this code on Windows. That's not true as I stated in an earlier review. I have some ideas on how to handle this (given existing positive AND negative tests), but I need to distribute something and get buy-in from the community. Until then, this isn't clear. As for as specifically handling chmod, I'd like to completely change the interface into something that is Mesos-specific (there are only a few use cases of this in Mesos today), rather than specifying Linux flags. That will be sure to keep both Linux and Windows in-sync with one another. That's what this bug is all about. If we need to chmod code and deal with ownership, we should refactor that into what Mesos actually needs and uses, and not be based on Linux specifically, since Mesos runs on more than just Linux now. My vote is to leave this as is. When MESOS-3176 is resolved, this code will be fixed in a permanent way. > On Oct. 18, 2017, 1:36 a.m., Andrew Schwartzmeyer wrote: > > src/launcher/fetcher.cpp > > Lines 482-489 (original), 483-494 (patched) > > <https://reviews.apache.org/r/60628/diff/4/?file=1858875#file1858875line491> > > > > This one is `chown`, not `chmod`, and really more of a revisit of > > MESOS-4310 (and MESOS-8063). It is possible to impersonate a user on > > Windows, so we probably should open a new issue to implement `chown` and > > then `runas` like code-paths. A runas like code path is much like su. I've updated the comment to reer to chown and MESOS-8063. I think this can be handled with that change, most likely. If not, we can create a new issue at that time. > On Oct. 18, 2017, 1:36 a.m., Andrew Schwartzmeyer wrote: > > src/tests/fetcher_tests.cpp > > Line 531 (original), 535 (patched) > > <https://reviews.apache.org/r/60628/diff/4/?file=1858878#file1858878line535> > > > > Glad this worked out :) Me too! I was happy to see the test pass. > On Oct. 18, 2017, 1:36 a.m., Andrew Schwartzmeyer wrote: > > src/tests/fetcher_tests.cpp > > Lines 925 (patched) > > <https://reviews.apache.org/r/60628/diff/4/?file=1858878#file1858878line925> > > > > Nit: s/doesns't/doesn't/ Ok. - Jeff ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60628/#review188433 ----------------------------------------------------------- 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/60628/ > ----------------------------------------------------------- > > (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 > ------- > > Tests for tar, gzip, and such won't be working on Windows for > the time being. Thoughts are to provide this capability to the > Fetcher in a cross-platform manner via a programmatic code library > rather than Linux-specific command line tools (tar, gzip, etc). > See MESOS-8064 for more details. > > In the short term, zip and unzip will work since PowerShell can > support that natively. > > > Diffs > ----- > > src/launcher/CMakeLists.txt c7a83d476efe13d65fa529e7676b6488eb48f44b > src/launcher/fetcher.cpp 5a9e93adbf2e4f0b8ff64a69e62fea5f6a0b8f2d > src/slave/containerizer/fetcher.cpp > ba5b0979aa427c5b3dbacf39661d6027da13e0d6 > src/tests/CMakeLists.txt 386e0473c93d0a993248c7818067071d0c761c76 > src/tests/fetcher_tests.cpp df9d2d9586a6457004506c4e2a972ccfc912c7c5 > > > Diff: https://reviews.apache.org/r/60628/diff/4/ > > > Testing > ------- > > See upstream. > > > Thanks, > > Jeff Coffler > >
