mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joseph Wu <jos...@mesosphere.io>
Subject Re: Review Request 54336: Windows: Fix `Flags::runtime_dir` value.
Date Tue, 06 Dec 2016 21:49:47 GMT


> On Dec. 5, 2016, 6:33 p.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.
> 
> Alex Clemmer wrote:
>     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?
> 
> Alex Clemmer wrote:
>     Oh, also, on the two points above, it's worth reading my next comment to see a more
complete picture for our current plan to accomplish the second part in particular.
> 
> Andrew Schwartzmeyer wrote:
>     I would like to point out that, as far as I can tell, this is the only hardcoded
absolute path in any of the `constants.hpp` headers. I think that it should be removed as
a matter of preventing its accidental use on a platform for which it is incorrect.
>     
>     That said, I can see why you'd be not want to remove it given just this commit, as
it only moves the definition into the `Flags::runtime_dir`, which is *also* not the right
place for this. Perhaps it'd be appropriate to add `os::runstate_dir` as Alex and Jie suggest
to hold this platform-depenent constant (keeping in mind that this commit also does not introduce
the correct Windows location of `ProgramData`).

Ok, sounds like a reasonable workaround for now.


- Joseph


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


On Dec. 5, 2016, 5:06 p.m., Andrew Schwartzmeyer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54336/
> -----------------------------------------------------------
> 
> (Updated Dec. 5, 2016, 5:06 p.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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message