> On Dec. 6, 2016, 2:33 a.m., Joseph Wu wrote: > > src/slave/constants.hpp, lines 141-144 > > <https://reviews.apache.org/r/54336/diff/5/?file=1577361#file1577361line141> > > > > There isn't any need to get rid of this constant, but we could update > > the comment. > > > > i.e. This is the _desired_ default, but if the path is inaccessible > > (posix non-root), the default will be a temporary folder.
I have a different view, actually. To me it seems like there's no particular reason to have it, while there _are_ in my view a few good reasons to not have it (and, I tend to believe that you should have to justify why something exists rather than why it shouldn't exist). And, since it is used only once, it costs nothing to change. * We should really try to keep path literals out of the codebase as much as possible, because of the subtle bugs and problems with combining paths with '\' and '/' on Windows. Even if it is safe in this case to use, I think we should always think carefully about whether we have a good reason for having a string literal path, and in this case, I don't see a clear benefit, while I do see a clear risk of someone accidentally `path::join`'ing onto the end at some point in the future. It seems like a better choice (as Jie suggests in #54335) would be to implement this as `os::runstatedir`. See my next point for more on this. * To me, it makes sense to have a platform-indepenent variable data root in a new function `os::var()` (_i.e._ `/var` on POSIX, and something like `ProgramData` on Windows), and to use something like `path::join(os::var(), ...)` to implement a function, `os::runstatedir()`. I think this is reasonable in particular since (per conversation with Jie) we don't want to change the default of this path away from `/var`. What are your thoughts? Am I missing something? > On Dec. 6, 2016, 2:33 a.m., Joseph Wu wrote: > > src/slave/flags.cpp, lines 214-216 > > <https://reviews.apache.org/r/54336/diff/5/?file=1577362#file1577362line214> > > > > Seems like the problem on Windows is `os::user()` rather than the value > > of the `--runtime_dir` flag. As long as `os::user()` returns some value, > > we'll get an appropriate default runtime directory for Windows. In most > > cases, `/var/run/mesos` should work on Windows (unless there are permission > > issues?), because our code does a recursive `mkdir` before using the > > runtime directory. > > > > If this is the case, this review is probably the approach we want to > > take: https://reviews.apache.org/r/53706/ I actually don't see it this way. As I said in the comments of #54335, the reason we are switching on the `os::user` at all is because `/var` is accessible only from `root` in some POSIXes. Since Windows does not suffer from this problem, I think we should put it in a place that is sensible for Windows, which should eventually be something like `path::join(os::var(), "mesos", "runtime")`. Until we have `os::var` (sometime in the next couple days), I think it's reasonable to put this data in the default non-`root` Unix folder to unblock the rest of the Windows team. What do you think? - Alex ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54336/#review158103 ----------------------------------------------------------- On Dec. 6, 2016, 1:06 a.m., Andrew Schwartzmeyer wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54336/ > ----------------------------------------------------------- > > (Updated Dec. 6, 2016, 1:06 a.m.) > > > Review request for mesos and Alex Clemmer. > > > Bugs: MESOS-6677 > https://issues.apache.org/jira/browse/MESOS-6677 > > > Repository: mesos > > > Description > ------- > > This commit fixes MESOS-6677, which breaks the ability > to run any agent on Windows, and thus is blocking all > Windows development progress on the `master` branch. > > The cause was that the default `runtime_dir` value was POSIX specific, > and used `os::user()` which is deleted on Windows. > The fix is to guard the POSIX code, and add a > Windows implementation. > > > Diffs > ----- > > src/slave/constants.hpp 6c381f06365b9deb84f43cdd101a2d2e5d826f57 > src/slave/flags.cpp 0de15eca7da9bf8fbdbb90c6e96edfe76f4a0f44 > > Diff: https://reviews.apache.org/r/54336/diff/ > > > Testing > ------- > > make && make check on Linux: 1411 tests passed, no failures. > > msbuild and attached to Linux master: no runtime failures. > > > Thanks, > > Andrew Schwartzmeyer > >
