Re: Review Request 66046: CMake: Patched Boost to remove spurious warnings.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66046/#review199215 --- Ship it! Ship It! - Jeff Coffler On March 13, 2018, 10:30 p.m., Andrew Schwartzmeyer wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66046/ > --- > > (Updated March 13, 2018, 10:30 p.m.) > > > Review request for mesos, Akash Gupta, Benjamin Bannier, Jeff Coffler, John > Kordich, Joseph Wu, and Michael Park. > > > Bugs: MESOS-8556 > https://issues.apache.org/jira/browse/MESOS-8556 > > > Repository: mesos > > > Description > --- > > On Windows, Boost explicitly checks if it's being compiled with a > compiler version they've tested. As we tend to update Visual Studio > whenever a stable update is available, this leads to a Boost warning, > "Unknown compiler version..." being emitted repeatedly (for every file > which includes a Boost header). > > If it were emitted once, we would leave it be, but because it's > repeated hundreds of times, and is mostly harmless, we patch the build > to remove the warning itself. Unfortunately, there is no compile-time > option to disable the warning instead of a patch. > > Note that it is not straight-forward to generate a patch file for > Boost. The steps were: clone the Boost repo recursively, go the > `libs/config` submodule, edit the `visualc.hpp` file, go to the > `include` subdirectory, and use `git diff --relative` to generate the > patch with the corrects paths expected for the extracted tarball. > > > Diffs > - > > 3rdparty/CMakeLists.txt 61dc788edf96ca6e8e3ee736eb232c0c118e6191 > 3rdparty/boost-1.65.0.patch PRE-CREATION > > > Diff: https://reviews.apache.org/r/66046/diff/1/ > > > Testing > --- > > Built on Windows; no more warning. > > > Thanks, > > Andrew Schwartzmeyer > >
Re: Review Request 66046: CMake: Patched Boost to remove spurious warnings.
> On March 14, 2018, 6:44 p.m., Andrew Schwartzmeyer wrote:
> > So, this patch is fairly temporary. The tip of `master` of Boost (of that
> > submodule) has this commit:
> >
> > ```
> > commit 5ad073063 (origin/develop, origin/HEAD, develop)
> > Author: jzmaddock
> > Date: Wed Mar 7 18:02:01 2018 +
> >
> > visualc.hpp: Disable warning about outdated config.
> >
> > diff --git a/include/boost/config/compiler/visualc.hpp
> > b/include/boost/config/compiler/visualc.hpp
> > index 748d14076..c533c50df 100644
> > --- a/include/boost/config/compiler/visualc.hpp
> > +++ b/include/boost/config/compiler/visualc.hpp
> > @@ -346,6 +346,9 @@
> > # if defined(BOOST_ASSERT_CONFIG)
> > # error "Boost.Config is older than your current compiler version."
> > # elif !defined(BOOST_CONFIG_SUPPRESS_OUTDATED_MESSAGE)
> > - BOOST_PRAGMA_MESSAGE("Info: Boost.Config is older than your compiler
> > version - probably nothing bad will happen - but you may wish to look for
> > an updated Boost vers
> > ion. Define BOOST_CONFIG_SUPPRESS_OUTDATED_MESSAGE to suppress this
> > message.")
> > + //
> > + // Disabled as of March 2018 - the pace of VS releases is hard to
> > keep up with
> > + // and in any case, we have relatively few defect macros defined now.
> > + // BOOST_PRAGMA_MESSAGE("Info: Boost.Config is older than your
> > compiler version - probably nothing bad will happen - but you may wish to
> > look for an updated Boost v
> > ersion. Define BOOST_CONFIG_SUPPRESS_OUTDATED_MESSAGE to suppress this
> > message.")
> > # endif
> > #endif
> > ```
> >
> > So when we get an updated version of Boost with this change, we can just
> > add `BOOST_CONFIG_SUPPRESS_OUTDATED_MESSAGE` to the pre-processor
> > definitions and delete this patch. But it is necessary for now (and even
> > upstream they realized this code was awful).
With this in mind, I'm fine with the patch.
- Jeff
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66046/#review199198
---
On March 13, 2018, 10:30 p.m., Andrew Schwartzmeyer wrote:
>
> -------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66046/
> ---
>
> (Updated March 13, 2018, 10:30 p.m.)
>
>
> Review request for mesos, Akash Gupta, Benjamin Bannier, Jeff Coffler, John
> Kordich, Joseph Wu, and Michael Park.
>
>
> Bugs: MESOS-8556
> https://issues.apache.org/jira/browse/MESOS-8556
>
>
> Repository: mesos
>
>
> Description
> ---
>
> On Windows, Boost explicitly checks if it's being compiled with a
> compiler version they've tested. As we tend to update Visual Studio
> whenever a stable update is available, this leads to a Boost warning,
> "Unknown compiler version..." being emitted repeatedly (for every file
> which includes a Boost header).
>
> If it were emitted once, we would leave it be, but because it's
> repeated hundreds of times, and is mostly harmless, we patch the build
> to remove the warning itself. Unfortunately, there is no compile-time
> option to disable the warning instead of a patch.
>
> Note that it is not straight-forward to generate a patch file for
> Boost. The steps were: clone the Boost repo recursively, go the
> `libs/config` submodule, edit the `visualc.hpp` file, go to the
> `include` subdirectory, and use `git diff --relative` to generate the
> patch with the corrects paths expected for the extracted tarball.
>
>
> Diffs
> -
>
> 3rdparty/CMakeLists.txt 61dc788edf96ca6e8e3ee736eb232c0c118e6191
> 3rdparty/boost-1.65.0.patch PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/66046/diff/1/
>
>
> Testing
> ---
>
> Built on Windows; no more warning.
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 66002: Fixed the HTTP API path variables on Windows.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66002/#review199213 --- Ship it! Ship It! - Jeff Coffler On March 9, 2018, 8:17 p.m., John Kordich wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66002/ > --- > > (Updated March 9, 2018, 8:17 p.m.) > > > Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, Jeff Coffler, > and Joseph Wu. > > > Repository: mesos > > > Description > --- > > Many mesos frameworks assume that path separators are forward slashes, > like dcos-ui and marathon-ui. dcos-cli also assumes this. Previous to > this patch, if a forward slash was given in the path variable to an HTTP > API function call to the mesos agent on a Windows system, the Windows > API would fail at recognizing the path, because the Windows API accepts > only backslashes as path separators. To remedy this issue, we now > convert all forward slashes passed as a path to the HTTP API to an agent > to back slashes on Windows agents by using the path::from_uri function. > > > Diffs > - > > src/files/files.cpp 5f92d2af722c3201200fa1d6a75dc0b87fdc6078 > src/tests/files_tests.cpp c703cae03345112715aeab83cb0a74abe3e12469 > > > Diff: https://reviews.apache.org/r/66002/diff/2/ > > > Testing > --- > > Tested on both Windows and Linux, all tests pass, including the two newly > enabled tests on Windows. > > > Thanks, > > John Kordich > >
Re: Review Request 66046: CMake: Patched Boost to remove spurious warnings.
> On March 14, 2018, 12:21 a.m., Jeff Coffler wrote: > > 3rdparty/boost-1.65.0.patch > > Lines 12 (patched) > > <https://reviews.apache.org/r/66046/diff/1/?file=1974924#file1974924line12> > > > > I'm thinking the other way around. That is, if the compiler version < > > 1910, issue the warning, otherwise be quiet. > > > > That way: > > > > 1. We only update this when we update minimum compiler version, > > 2. Greater versions are legal, as long as it's >= the minimum compiler > > version. > > > > The way you have it now, we'd need to update this every single time a > > new compiler came out. Yuck. I think we only care if new compiler is > > REQUIRED for some reason. > > Andrew Schwartzmeyer wrote: > That's not quite what's going on here. The upstream code is checking if > `_MSC_VER > 1910`, that is, "is the compiler newer than what we've tested > (1910)? If so, emit this warning." But we compile with, for instance, MSVC > 1912 (latest dot-update to VS). > > But look closer, this is a new patch file _removing_ this code because > the check is causing _way_ too many warnings. No, Boost, you did not break > between 1910 and 1912; stop warning us. I understand that. I agree the current code is broken. My question: If we're using a "less than supported" version of the compiler, isn't this a GOOD error to get? Or is this something that cmake checks for? I guess, what I'm asking here: What happens if my version of Visual Studio is too low? Do we support ALL versions of Visual Studio 2017? Or do we need patch levels? If we do need a patch level, I think we should make it obvious by checking for that here (or by aborting during the cmake step). - Jeff --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66046/#review199133 --- On March 13, 2018, 10:30 p.m., Andrew Schwartzmeyer wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66046/ > --- > > (Updated March 13, 2018, 10:30 p.m.) > > > Review request for mesos, Akash Gupta, Benjamin Bannier, Jeff Coffler, John > Kordich, Joseph Wu, and Michael Park. > > > Bugs: MESOS-8556 > https://issues.apache.org/jira/browse/MESOS-8556 > > > Repository: mesos > > > Description > --- > > On Windows, Boost explicitly checks if it's being compiled with a > compiler version they've tested. As we tend to update Visual Studio > whenever a stable update is available, this leads to a Boost warning, > "Unknown compiler version..." being emitted repeatedly (for every file > which includes a Boost header). > > If it were emitted once, we would leave it be, but because it's > repeated hundreds of times, and is mostly harmless, we patch the build > to remove the warning itself. Unfortunately, there is no compile-time > option to disable the warning instead of a patch. > > Note that it is not straight-forward to generate a patch file for > Boost. The steps were: clone the Boost repo recursively, go the > `libs/config` submodule, edit the `visualc.hpp` file, go to the > `include` subdirectory, and use `git diff --relative` to generate the > patch with the corrects paths expected for the extracted tarball. > > > Diffs > - > > 3rdparty/CMakeLists.txt 61dc788edf96ca6e8e3ee736eb232c0c118e6191 > 3rdparty/boost-1.65.0.patch PRE-CREATION > > > Diff: https://reviews.apache.org/r/66046/diff/1/ > > > Testing > --- > > Built on Windows; no more warning. > > > Thanks, > > Andrew Schwartzmeyer > >
Re: Review Request 66046: CMake: Patched Boost to remove spurious warnings.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66046/#review199133 --- 3rdparty/boost-1.65.0.patch Lines 12 (patched) <https://reviews.apache.org/r/66046/#comment279439> I'm thinking the other way around. That is, if the compiler version < 1910, issue the warning, otherwise be quiet. That way: 1. We only update this when we update minimum compiler version, 2. Greater versions are legal, as long as it's >= the minimum compiler version. The way you have it now, we'd need to update this every single time a new compiler came out. Yuck. I think we only care if new compiler is REQUIRED for some reason. - Jeff Coffler On March 13, 2018, 10:30 p.m., Andrew Schwartzmeyer wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66046/ > --- > > (Updated March 13, 2018, 10:30 p.m.) > > > Review request for mesos, Akash Gupta, Benjamin Bannier, Jeff Coffler, John > Kordich, Joseph Wu, and Michael Park. > > > Bugs: MESOS-8556 > https://issues.apache.org/jira/browse/MESOS-8556 > > > Repository: mesos > > > Description > --- > > On Windows, Boost explicitly checks if it's being compiled with a > compiler version they've tested. As we tend to update Visual Studio > whenever a stable update is available, this leads to a Boost warning, > "Unknown compiler version..." being emitted repeatedly (for every file > which includes a Boost header). > > If it were emitted once, we would leave it be, but because it's > repeated hundreds of times, and is mostly harmless, we patch the build > to remove the warning itself. Unfortunately, there is no compile-time > option to disable the warning instead of a patch. > > Note that it is not straight-forward to generate a patch file for > Boost. The steps were: clone the Boost repo recursively, go the > `libs/config` submodule, edit the `visualc.hpp` file, go to the > `include` subdirectory, and use `git diff --relative` to generate the > patch with the corrects paths expected for the extracted tarball. > > > Diffs > - > > 3rdparty/CMakeLists.txt 61dc788edf96ca6e8e3ee736eb232c0c118e6191 > 3rdparty/boost-1.65.0.patch PRE-CREATION > > > Diff: https://reviews.apache.org/r/66046/diff/1/ > > > Testing > --- > > Built on Windows; no more warning. > > > Thanks, > > Andrew Schwartzmeyer > >
Re: Review Request 65936: Changed flags to CreateFile to support Windows symlink path resolution.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65936/#review198904 --- Ship it! Ship It! - Jeff Coffler On March 8, 2018, 7:22 p.m., John Kordich wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/65936/ > --- > > (Updated March 8, 2018, 7:22 p.m.) > > > Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, Jeff Coffler, > and Joseph Wu. > > > Bugs: MESOS-8646 > https://issues.apache.org/jira/browse/MESOS-8646 > > > Repository: mesos > > > Description > --- > > Changed flags to CreateFile to support Windows symlink path resolution. > > > Diffs > - > > 3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp > 858b3c7c27e95b9cf46a7457c1aaa05a0e19188d > 3rdparty/stout/tests/os/filesystem_tests.cpp > b84f1ae17246d94947538efeaf504a2cd247f20e > > > Diff: https://reviews.apache.org/r/65936/diff/2/ > > > Testing > --- > > I encountered a permissions error stating "The process cannot access the file > because it is being used by another process." when attempting to download > stderr/stdout from a task running on a Windows agent, or when attempting to > download the mesos agent log on a Windows agent. > > After some very helpful information from Andy and Akash, I made this change > and confirmed it fixed this issue with the task/agent logs. > > I also ran the automated tests on Windows and all tests pass. > > I did not test this on Linux, but because this file is compiled only on > Windows, it shouldn't be necessary. > > > Thanks, > > John Kordich > >
Re: Review Request 65872: Windows: Fixed location of Docker's `config.json` file.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65872/#review198488
---
Ship it!
Ship It!
- Jeff Coffler
On March 1, 2018, 11:57 p.m., Andrew Schwartzmeyer wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65872/
> ---
>
> (Updated March 1, 2018, 11:57 p.m.)
>
>
> Review request for mesos, Akash Gupta, Jeff Coffler, and Joseph Wu.
>
>
> Bugs: MESOS-8619
> https://issues.apache.org/jira/browse/MESOS-8619
>
>
> Repository: mesos
>
>
> Description
> ---
>
> Per MESOS-8619, Docker checks `$USERPROFILE/.docker/config.json`
> instead of `$HOME`. Mesos overrides this environment variable in order
> to point Docker to a `config.json` file in another location, so we
> have to fix the assumption we made about Docker.
>
> We do not add this constant to stout, because it is not consistent
> across Windows applications. This particular logic is specific to the
> implementation of Docker. Other applications might check `$HOME` or
> `$HOMEPATH` on Windows.
>
>
> Diffs
> -
>
> src/docker/docker.cpp 876dfffc2ee68e345ff336fefa6cf908c3d2a5c4
>
>
> Diff: https://reviews.apache.org/r/65872/diff/1/
>
>
> Testing
> ---
>
> Deployed a `docker.zip` consisting of `.docker/config.json` in "Linux-form"
> i.e. with base64 encoded password (not `wincred`) using the following task:
>
> ```
> {
> "name": "fetcher-test",
> "task_id": {"value" : "fetcher"},
> "agent_id": {"value" : ""},
> "resources": [
> {
> "name": "cpus",
> "type": "SCALAR",
> "scalar": {
> "value": 1
> }
> },
> {
> "name": "mem",
> "type": "SCALAR",
> "scalar": {
> "value": 512
> }
> }
> ],
> "command": {
> "uris": [ {"value": "file://C:/Users/andschwa/docker.zip"} ],
> "shell": false
> },
> "container": {
> "type": "DOCKER",
> "docker": {"image": "andschwa/nanoserver:1709"}
> }
> }
> ```
>
> The `andschwa/nanoserver:1709` is a _private_ repo, and `docker logout` was
> run (and confirmed that the machine could not pull the image manually).
>
> With this patch and the `docker.zip` URI, it was fetched, unzipped, and found
> by `docker pull`, enabling it to successfully pull the private image.
>
> ```
> > docker images
> REPOSITORYTAG IMAGE IDCREATED
> SIZE
> andschwa/nanoserver 1709816017814fa22 weeks ago
> 312MB
> ```
>
> This is difficult to unit-test because it requires private credentials, an
> external private docker repo, and global side effects of Docker images (i.e.
> you can't have it cached). But I am, of course, open to ideas of how to
> programmatically test this.
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 65624: Added fetcher tests for `stderr` output.
> On Feb. 20, 2018, 7:43 p.m., Jeff Coffler wrote: > > src/tests/fetcher_tests.cpp > > Lines 132 (patched) > > <https://reviews.apache.org/r/65624/diff/1/?file=1958439#file1958439line132> > > > > These tests pretty much check the fetcher in a "stand-alone" > > environment. Could you add some tests to check within a container as well, > > particularly since fetcher logging is slightly different within a container? > > > > Note that while fetcher output is slightly different within a > > container, fetcher itself isn't (docker is sharing the same directory). But > > we don't have any sort of tests to validate that - that I'm aware of anyway. > > Andrew Schwartzmeyer wrote: > This covers the affected scenarios; nothing changes running "in a > container" on Windows compared to this unit test with respect to output > redirection. When task is started with a URI to fetch, the fetcher is > directly invoked with the sandbox directory as done here to fetch the URI(s), > before the container starts. In essence, the fetcher is always run > "stand-alone". That's correct for fetching. However, logging is somewhat different since stderr/stdout logging is somewhat different under Docker vs. on a stand-alone system. That's what I was referring to ... - Jeff --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65624/#review197799 --- On Feb. 20, 2018, 7:40 p.m., Andrew Schwartzmeyer wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/65624/ > --- > > (Updated Feb. 20, 2018, 7:40 p.m.) > > > Review request for mesos, Akash Gupta, Jeff Coffler, Jie Yu, and Joseph Wu. > > > Repository: mesos > > > Description > --- > > The fetcher is supposed to log its `stderr` output to a redirected file > called `stderr` in the given sandbox directory. There previously existed > a bug on Windows due to incorrect handle inheritance where this > redirection failed silently, leaving the log empty. These unit tests > assert that the correct content is logged to the `stderr` file for both > a success and failure fetch scenario. > > > Diffs > - > > src/tests/fetcher_tests.cpp 1b4ba1d286f218731c13180e5189e91f90e3d63f > > > Diff: https://reviews.apache.org/r/65624/diff/1/ > > > Testing > --- > > Windows (Linux pending): > ``` > [ RUN ] FetcherTest.LogSuccessToStderr > [ OK ] FetcherTest.LogSuccessToStderr (197 ms) > [ RUN ] FetcherTest.LogFailureToStderr > [ OK ] FetcherTest.LogFailureToStderr (320 ms) > ``` > > > Thanks, > > Andrew Schwartzmeyer > >
Re: Review Request 65624: Added fetcher tests for `stderr` output.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65624/#review197799 --- src/tests/fetcher_tests.cpp Lines 132 (patched) <https://reviews.apache.org/r/65624/#comment278074> These tests pretty much check the fetcher in a "stand-alone" environment. Could you add some tests to check within a container as well, particularly since fetcher logging is slightly different within a container? Note that while fetcher output is slightly different within a container, fetcher itself isn't (docker is sharing the same directory). But we don't have any sort of tests to validate that - that I'm aware of anyway. - Jeff Coffler On Feb. 20, 2018, 7:40 p.m., Andrew Schwartzmeyer wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/65624/ > --- > > (Updated Feb. 20, 2018, 7:40 p.m.) > > > Review request for mesos, Akash Gupta, Jeff Coffler, Jie Yu, and Joseph Wu. > > > Repository: mesos > > > Description > --- > > The fetcher is supposed to log its `stderr` output to a redirected file > called `stderr` in the given sandbox directory. There previously existed > a bug on Windows due to incorrect handle inheritance where this > redirection failed silently, leaving the log empty. These unit tests > assert that the correct content is logged to the `stderr` file for both > a success and failure fetch scenario. > > > Diffs > - > > src/tests/fetcher_tests.cpp 1b4ba1d286f218731c13180e5189e91f90e3d63f > > > Diff: https://reviews.apache.org/r/65624/diff/1/ > > > Testing > --- > > Windows (Linux pending): > ``` > [ RUN ] FetcherTest.LogSuccessToStderr > [ OK ] FetcherTest.LogSuccessToStderr (197 ms) > [ RUN ] FetcherTest.LogFailureToStderr > [ OK ] FetcherTest.LogFailureToStderr (320 ms) > ``` > > > Thanks, > > Andrew Schwartzmeyer > >
Re: Review Request 65469: Windows: Updated `internal::process:createChildProcess`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65469/#review197798 --- Ship it! Ship It! - Jeff Coffler On Feb. 20, 2018, 7:40 p.m., Andrew Schwartzmeyer wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/65469/ > --- > > (Updated Feb. 20, 2018, 7:40 p.m.) > > > Review request for mesos, Akash Gupta, Jeff Coffler, Jie Yu, and Joseph Wu. > > > Bugs: MESOS-8512 > https://issues.apache.org/jira/browse/MESOS-8512 > > > Repository: mesos > > > Description > --- > > The interface of `internal::windows:create_process` was changed to > accept a `std::array` instead of a `std::tuple`. > > Furthermore, the error handling in `subprocess` was incorrect. We only > need to check the success of `createChildProcess`; we do not need to > check if `pid == -1` on Windows. We also now return the actual error > from `create_process` if it errored, instead of the whole > `Try`. > > The TODO about closing the child-ends of the file descriptors was > resolved. We now close these in `createChildProcess`, immediately after > `create_process`. This means we only have to do it once. > > > Diffs > - > > 3rdparty/libprocess/src/subprocess.cpp > 785e2e1083d115d25fffde2df74ed8bc1726511c > 3rdparty/libprocess/src/subprocess_windows.hpp > 0183bb451f68528acf31ed97754320c64f35102b > > > Diff: https://reviews.apache.org/r/65469/diff/2/ > > > Testing > --- > > > Thanks, > > Andrew Schwartzmeyer > >
Re: Review Request 65467: Windows: Added `internal::windows::set_inherit(WindowsFD, bool)`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65467/#review197796 --- Ship it! Ship It! - Jeff Coffler On Feb. 8, 2018, 10:35 p.m., Andrew Schwartzmeyer wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/65467/ > --- > > (Updated Feb. 8, 2018, 10:35 p.m.) > > > Review request for mesos, Akash Gupta, Jie Yu, and Joseph Wu. > > > Bugs: MESOS-8512 > https://issues.apache.org/jira/browse/MESOS-8512 > > > Repository: mesos > > > Description > --- > > This function is used on Windows to explicitly enable or disable > inheritance on a handle. > > > Diffs > - > > 3rdparty/stout/include/stout/internal/windows/inherit.hpp PRE-CREATION > > > Diff: https://reviews.apache.org/r/65467/diff/2/ > > > Testing > --- > > > Thanks, > > Andrew Schwartzmeyer > >
Re: Review Request 65403: Windows: Disabled `O_CLOEXEC` semantic mapping on Windows.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65403/#review197795 --- Ship it! Ship It! - Jeff Coffler On Feb. 8, 2018, 10:36 p.m., Andrew Schwartzmeyer wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/65403/ > --- > > (Updated Feb. 8, 2018, 10:36 p.m.) > > > Review request for mesos, Akash Gupta, Jie Yu, and Joseph Wu. > > > Bugs: MESOS-5882 > https://issues.apache.org/jira/browse/MESOS-5882 > > > Repository: mesos > > > Description > --- > > Previously we mapped `O_CLOEXEC` to `O_NOINHERIT`, which is close to the > intended semantics, but not close enough. Instead, we want all Windows > handles by default to be non-inheritable, and not tie any Windows > inheritance semantics to the close-on-exec semantics. So, we define > `O_CLOEXEC` on Windows to be `0`, and remove the logging from the > `os::cloexec` family of functions because do not ever intend to > implement them. > > We achieve the "close" part of the semantics by making all handles > non-inheritable, as the intent is to avoid leaking handles. > > > Diffs > - > > 3rdparty/stout/include/stout/os/open.hpp > 6711f22d44b0e7c13da3abd51f9fb7b71779e868 > 3rdparty/stout/include/stout/os/windows/fcntl.hpp > 5800ec92f85401a80cb813afd880be2e5a24a3af > > > Diff: https://reviews.apache.org/r/65403/diff/2/ > > > Testing > --- > > > Thanks, > > Andrew Schwartzmeyer > >
Re: Review Request 65617: Windows: Fixed problems with Hadoop URI handling on Windows.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65617/ --- (Updated Feb. 13, 2018, 4:52 p.m.) Review request for mesos, Andrew Schwartzmeyer and John Kordich. Bugs: MESOS-6715 https://issues.apache.org/jira/browse/MESOS-6715 Repository: mesos Description (updated) --- Fixed Windows-specific issues (absolute path, subprocess invocation) to allow Hadoop Fetcher Plugin to run properly. Diffs - src/hdfs/hdfs.cpp f9fc1cbb5556d4c1b2ee3a103b4f3bb4fe1d7589 Diff: https://reviews.apache.org/r/65617/diff/2/ Testing --- Full test pass on both Linux and Windows. Additionally, on Windows platform: ``` [==] Running 3 tests from 1 test case. [--] Global test environment set-up. [--] 3 tests from HadoopFetcherPluginTest [ RUN ] HadoopFetcherPluginTest.FetchExistingFile C:\Users\jeffcof\AppData\Local\Temp\UrtxvI>if "version" == "version" (exit 0 ) [ OK ] HadoopFetcherPluginTest.FetchExistingFile (235 ms) [ RUN ] HadoopFetcherPluginTest.FetchNonExistingFile C:\Users\jeffcof\AppData\Local\Temp\YJVcTA>if "version" == "version" (exit 0 ) [ OK ] HadoopFetcherPluginTest.FetchNonExistingFile (217 ms) [ RUN ] HadoopFetcherPluginTest.InvokeFetchByName C:\Users\jeffcof\AppData\Local\Temp\wsQjH1>if "version" == "version" (exit 0 ) [ OK ] HadoopFetcherPluginTest.InvokeFetchByName (217 ms) [--] 3 tests from HadoopFetcherPluginTest (674 ms total) [--] Global test environment tear-down [==] 3 tests from 1 test case ran. (12790 ms total) [ PASSED ] 3 tests. ``` Thanks, Jeff Coffler
Re: Review Request 65618: Windows: Enabled `curl` and `hadoop` fetcher plugins on Windows.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65618/ --- (Updated Feb. 13, 2018, 12:59 a.m.) Review request for mesos, Andrew Schwartzmeyer and John Kordich. Bugs: MESOS-6715 https://issues.apache.org/jira/browse/MESOS-6715 Repository: mesos Description --- Enabled both `curl` and `hadoop` fetcher plugins on Windows platform. Diffs (updated) - src/uri/fetcher.hpp 2b2b14ec0324d4d4dc12ac9c99fd74708fc83065 src/uri/fetcher.cpp 13c2d5499b6c551e1333a73e8473de308e6c3bc2 Diff: https://reviews.apache.org/r/65618/diff/2/ Changes: https://reviews.apache.org/r/65618/diff/1-2/ Testing --- Thanks, Jeff Coffler
Re: Review Request 65617: Windows: Fixed problems with Hadoop URI handling on Windows.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65617/ --- (Updated Feb. 13, 2018, 12:58 a.m.) Review request for mesos, Andrew Schwartzmeyer and John Kordich. Bugs: MESOS-6715 https://issues.apache.org/jira/browse/MESOS-6715 Repository: mesos Description (updated) --- Fix Windows-specific issues (absolute path, subprocess invocation) to allow Hadoop Fetcher Plugin to run properly. Diffs (updated) - src/hdfs/hdfs.cpp f9fc1cbb5556d4c1b2ee3a103b4f3bb4fe1d7589 Diff: https://reviews.apache.org/r/65617/diff/2/ Changes: https://reviews.apache.org/r/65617/diff/1-2/ Testing --- Full test pass on both Linux and Windows. Additionally, on Windows platform: ``` [==] Running 3 tests from 1 test case. [--] Global test environment set-up. [--] 3 tests from HadoopFetcherPluginTest [ RUN ] HadoopFetcherPluginTest.FetchExistingFile C:\Users\jeffcof\AppData\Local\Temp\UrtxvI>if "version" == "version" (exit 0 ) [ OK ] HadoopFetcherPluginTest.FetchExistingFile (235 ms) [ RUN ] HadoopFetcherPluginTest.FetchNonExistingFile C:\Users\jeffcof\AppData\Local\Temp\YJVcTA>if "version" == "version" (exit 0 ) [ OK ] HadoopFetcherPluginTest.FetchNonExistingFile (217 ms) [ RUN ] HadoopFetcherPluginTest.InvokeFetchByName C:\Users\jeffcof\AppData\Local\Temp\wsQjH1>if "version" == "version" (exit 0 ) [ OK ] HadoopFetcherPluginTest.InvokeFetchByName (217 ms) [--] 3 tests from HadoopFetcherPluginTest (674 ms total) [--] Global test environment tear-down [==] 3 tests from 1 test case ran. (12790 ms total) [ PASSED ] 3 tests. ``` Thanks, Jeff Coffler
Re: Review Request 65620: Windows: Enabled `HadoopFetcherPlugin.*` tests on Windows platform.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65620/ --- (Updated Feb. 13, 2018, 12:58 a.m.) Review request for mesos, Andrew Schwartzmeyer and John Kordich. Bugs: MESOS-6715 https://issues.apache.org/jira/browse/MESOS-6715 Repository: mesos Description --- On Windows, used .bat (cmd.exe) script to emulate hadoop (rather than a shell script). Tested all `HadoopFetcherPlugin.*` tests, all succeed. Diffs (updated) - src/tests/uri_fetcher_tests.cpp 0acb8545b390a9b641d774cf65803d97c6296135 Diff: https://reviews.apache.org/r/65620/diff/2/ Changes: https://reviews.apache.org/r/65620/diff/1-2/ Testing --- Thanks, Jeff Coffler
Re: Review Request 65619: Windows: Modified build system to build hadoop plugins.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65619/ --- (Updated Feb. 13, 2018, 12:57 a.m.) Review request for mesos, Andrew Schwartzmeyer and John Kordich. Bugs: MESOS-6715 https://issues.apache.org/jira/browse/MESOS-6715 Repository: mesos Description (updated) --- Windows: Modified build system to build hadoop plugins. Diffs (updated) - src/CMakeLists.txt 21fb47e29dd0b19681690b8de5261c68b574a7c8 Diff: https://reviews.apache.org/r/65619/diff/2/ Changes: https://reviews.apache.org/r/65619/diff/1-2/ Testing --- Thanks, Jeff Coffler
Re: Review Request 65618: Windows: Enabled `curl` and `hadoop` fetcher plugins on Windows.
> On Feb. 13, 2018, 12:08 a.m., Andrew Schwartzmeyer wrote: > > I looked at MESOS-5473, and I had previously closed it because it didn't > > make sense. > > > > Would you open a new issue: "Enable Docker fetcher plugin on Windows" and > > update the comments here? Done, MESOS-8570: Enable Docker fetcher plugin on Windows. - Jeff --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65618/#review197335 ------- On Feb. 12, 2018, 11:31 p.m., Jeff Coffler wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/65618/ > --- > > (Updated Feb. 12, 2018, 11:31 p.m.) > > > Review request for mesos, Andrew Schwartzmeyer and John Kordich. > > > Bugs: MESOS-6715 > https://issues.apache.org/jira/browse/MESOS-6715 > > > Repository: mesos > > > Description > --- > > Enabled both `curl` and `hadoop` fetcher plugins on Windows platform. > > > Diffs > - > > src/uri/fetcher.hpp 2b2b14ec0324d4d4dc12ac9c99fd74708fc83065 > src/uri/fetcher.cpp 13c2d5499b6c551e1333a73e8473de308e6c3bc2 > > > Diff: https://reviews.apache.org/r/65618/diff/1/ > > > Testing > --- > > > Thanks, > > Jeff Coffler > >
Re: Review Request 65617: Windows: Fixed problems with Hadoop URI handling on Windows.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65617/ --- (Updated Feb. 12, 2018, 11:35 p.m.) Review request for mesos, Andrew Schwartzmeyer and John Kordich. Bugs: MESOS-6715 https://issues.apache.org/jira/browse/MESOS-6715 Repository: mesos Description --- Fixed Windows-specific issues (absolute path, subprocess invocation) to allow Hadoop Fetcher Plugin to run properly. Diffs - src/hdfs/hdfs.cpp f9fc1cbb5556d4c1b2ee3a103b4f3bb4fe1d7589 Diff: https://reviews.apache.org/r/65617/diff/1/ Testing (updated) --- Full test pass on both Linux and Windows. Additionally, on Windows platform: ``` [==] Running 3 tests from 1 test case. [--] Global test environment set-up. [--] 3 tests from HadoopFetcherPluginTest [ RUN ] HadoopFetcherPluginTest.FetchExistingFile C:\Users\jeffcof\AppData\Local\Temp\UrtxvI>if "version" == "version" (exit 0 ) [ OK ] HadoopFetcherPluginTest.FetchExistingFile (235 ms) [ RUN ] HadoopFetcherPluginTest.FetchNonExistingFile C:\Users\jeffcof\AppData\Local\Temp\YJVcTA>if "version" == "version" (exit 0 ) [ OK ] HadoopFetcherPluginTest.FetchNonExistingFile (217 ms) [ RUN ] HadoopFetcherPluginTest.InvokeFetchByName C:\Users\jeffcof\AppData\Local\Temp\wsQjH1>if "version" == "version" (exit 0 ) [ OK ] HadoopFetcherPluginTest.InvokeFetchByName (217 ms) [--] 3 tests from HadoopFetcherPluginTest (674 ms total) [--] Global test environment tear-down [==] 3 tests from 1 test case ran. (12790 ms total) [ PASSED ] 3 tests. ``` Thanks, Jeff Coffler
Review Request 65620: Windows: Enabled `HadoopFetcherPlugin.*` tests on Windows platform.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65620/ --- Review request for mesos, Andrew Schwartzmeyer and John Kordich. Bugs: MESOS-6715 https://issues.apache.org/jira/browse/MESOS-6715 Repository: mesos Description --- On Windows, used .bat (cmd.exe) script to emulate hadoop (rather than a shell script). Tested all `HadoopFetcherPlugin.*` tests, all succeed. Diffs - src/tests/uri_fetcher_tests.cpp 0acb8545b390a9b641d774cf65803d97c6296135 Diff: https://reviews.apache.org/r/65620/diff/1/ Testing --- Thanks, Jeff Coffler
Review Request 65619: Windows: Modified build system to build hadoop plugins.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65619/ --- Review request for mesos, Andrew Schwartzmeyer and John Kordich. Bugs: MESOS-6715 https://issues.apache.org/jira/browse/MESOS-6715 Repository: mesos Description --- Modified cmake build system to build hadoop plugins. Diffs - src/CMakeLists.txt 21fb47e29dd0b19681690b8de5261c68b574a7c8 Diff: https://reviews.apache.org/r/65619/diff/1/ Testing --- Thanks, Jeff Coffler
Review Request 65618: Windows: Enabled `curl` and `hadoop` fetcher plugins on Windows.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65618/ --- Review request for mesos, Andrew Schwartzmeyer and John Kordich. Bugs: MESOS-6715 https://issues.apache.org/jira/browse/MESOS-6715 Repository: mesos Description --- Enabled both `curl` and `hadoop` fetcher plugins on Windows platform. Diffs - src/uri/fetcher.hpp 2b2b14ec0324d4d4dc12ac9c99fd74708fc83065 src/uri/fetcher.cpp 13c2d5499b6c551e1333a73e8473de308e6c3bc2 Diff: https://reviews.apache.org/r/65618/diff/1/ Testing --- Thanks, Jeff Coffler
Review Request 65617: Windows: Fixed problems with Hadoop URI handling on Windows.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65617/ --- Review request for mesos, Andrew Schwartzmeyer and John Kordich. Bugs: MESOS-6715 https://issues.apache.org/jira/browse/MESOS-6715 Repository: mesos Description --- Fixed Windows-specific issues (absolute path, subprocess invocation) to allow Hadoop Fetcher Plugin to run properly. Diffs - src/hdfs/hdfs.cpp f9fc1cbb5556d4c1b2ee3a103b4f3bb4fe1d7589 Diff: https://reviews.apache.org/r/65617/diff/1/ Testing --- Thanks, Jeff Coffler
Re: Review Request 65510: Enabled `curl_uri_fetcher.cpp` tests on Windows platform.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65510/ --- (Updated Feb. 8, 2018, 11:13 p.m.) Review request for mesos, Andrew Schwartzmeyer and John Kordich. Summary (updated) - Enabled `curl_uri_fetcher.cpp` tests on Windows platform. Bugs: MESOS-6715 https://issues.apache.org/jira/browse/MESOS-6715 Repository: mesos Description (updated) --- These work now that `curl` is available on Windows. Diffs (updated) - src/tests/uri_fetcher_tests.cpp 8a40f46b215bb1f267a59a9edfca83445f86b430 Diff: https://reviews.apache.org/r/65510/diff/2/ Changes: https://reviews.apache.org/r/65510/diff/1-2/ Testing --- Thanks, Jeff Coffler
Re: Review Request 65508: Allowed curl program to be properly executed on Windows platform.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65508/ --- (Updated Feb. 8, 2018, 11:09 p.m.) Review request for mesos, Andrew Schwartzmeyer and John Kordich. Summary (updated) - Allowed curl program to be properly executed on Windows platform. Bugs: MESOS-6715 https://issues.apache.org/jira/browse/MESOS-6715 Repository: mesos Description (updated) --- On Windows, "curl.exe" must be executed rather then "curl". Diffs (updated) - src/uri/fetchers/curl.cpp f34daf2bc1f3fef74548e04ad332e3a627636bfd Diff: https://reviews.apache.org/r/65508/diff/2/ Changes: https://reviews.apache.org/r/65508/diff/1-2/ Testing --- Full build, unit tests on both Linux and Windows platform. Thanks, Jeff Coffler
Re: Review Request 65509: Fixed CURL_ prefix in unit test name to succeed on Windows.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65509/ --- (Updated Feb. 8, 2018, 11:09 p.m.) Review request for mesos, Andrew Schwartzmeyer and John Kordich. Summary (updated) - Fixed CURL_ prefix in unit test name to succeed on Windows. Bugs: MESOS-6715 https://issues.apache.org/jira/browse/MESOS-6715 Repository: mesos Description --- Since the 'curl.exe' executable is built on part of Windows (it's a build target), it will always exist for test purposes. Diffs (updated) - src/tests/environment.cpp 13a4c9514fcd3016fe623c597decd067457e86cd Diff: https://reviews.apache.org/r/65509/diff/2/ Changes: https://reviews.apache.org/r/65509/diff/1-2/ Testing --- Thanks, Jeff Coffler
Re: Review Request 65508: Allow curl program to be properly executed on Windows platform.
> On Feb. 6, 2018, 7:12 p.m., Andrew Schwartzmeyer wrote: > > Nit: Fix summary to be in past tense, and fix description to not be a copy > > of the summary. > > Jeff Coffler wrote: > This was a one-line commit, but I went ahead and added a second line. I'm > not sure why you said that the description shouldn't be a copy of the > summary, it wasn't. Tooling problem on your side, or just a mistake? > > Andrew Schwartzmeyer wrote: > When you post a review with just a summary, the scripts copy the summary > into the description. This unfortunately carries through to the other side > when the review is applied, and then you have a weird commit. The > (work-around?) is to not post one-line commit messages. > > Jeff Coffler wrote: > That's odd. I think you mentioned this before, but I see one-line commits > on Mesos, like this: > > ``` > commit 2ef5f4a > Author: Gilbert Song > Date: Thu Feb 8 08:45:42 2018 > > Added an image 1.3-1.5_v1_json_state_query_latency.png to docs/images. > > commit f7dbd29 > Author: Michael Park > Date: Wed Feb 7 20:28:42 2018 > > Updated ReviewBot to use Ubuntu 16.04. > ``` > > Am I doing something wrong. Do I need something "funny" in the > description (like a "." or something) if it's one line? Talked F2F with Andy. This is a tooling issue. One-line commits are possible, if after rbt, the copy of the one-line commit is removed from the "Description" field. - Jeff --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65508/#review196911 --- On Feb. 5, 2018, 6:12 p.m., Jeff Coffler wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/65508/ > --- > > (Updated Feb. 5, 2018, 6:12 p.m.) > > > Review request for mesos, Andrew Schwartzmeyer and John Kordich. > > > Bugs: MESOS-6715 > https://issues.apache.org/jira/browse/MESOS-6715 > > > Repository: mesos > > > Description > --- > > Allow curl program to be properly executed on Windows platform. > > > Diffs > ----- > > src/uri/fetchers/curl.cpp f34daf2bc1f3fef74548e04ad332e3a627636bfd > > > Diff: https://reviews.apache.org/r/65508/diff/1/ > > > Testing > --- > > Full build, unit tests on both Linux and Windows platform. > > > Thanks, > > Jeff Coffler > >
Re: Review Request 65508: Allow curl program to be properly executed on Windows platform.
> On Feb. 6, 2018, 7:12 p.m., Andrew Schwartzmeyer wrote: > > Nit: Fix summary to be in past tense, and fix description to not be a copy > > of the summary. > > Jeff Coffler wrote: > This was a one-line commit, but I went ahead and added a second line. I'm > not sure why you said that the description shouldn't be a copy of the > summary, it wasn't. Tooling problem on your side, or just a mistake? > > Andrew Schwartzmeyer wrote: > When you post a review with just a summary, the scripts copy the summary > into the description. This unfortunately carries through to the other side > when the review is applied, and then you have a weird commit. The > (work-around?) is to not post one-line commit messages. That's odd. I think you mentioned this before, but I see one-line commits on Mesos, like this: ``` commit 2ef5f4a Author: Gilbert Song Date: Thu Feb 8 08:45:42 2018 Added an image 1.3-1.5_v1_json_state_query_latency.png to docs/images. commit f7dbd29 Author: Michael Park Date: Wed Feb 7 20:28:42 2018 Updated ReviewBot to use Ubuntu 16.04. ``` Am I doing something wrong. Do I need something "funny" in the description (like a "." or something) if it's one line? - Jeff --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65508/#review196911 ------- On Feb. 5, 2018, 6:12 p.m., Jeff Coffler wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/65508/ > --- > > (Updated Feb. 5, 2018, 6:12 p.m.) > > > Review request for mesos, Andrew Schwartzmeyer and John Kordich. > > > Bugs: MESOS-6715 > https://issues.apache.org/jira/browse/MESOS-6715 > > > Repository: mesos > > > Description > --- > > Allow curl program to be properly executed on Windows platform. > > > Diffs > - > > src/uri/fetchers/curl.cpp f34daf2bc1f3fef74548e04ad332e3a627636bfd > > > Diff: https://reviews.apache.org/r/65508/diff/1/ > > > Testing > --- > > Full build, unit tests on both Linux and Windows platform. > > > Thanks, > > Jeff Coffler > >
Re: Review Request 65508: Allow curl program to be properly executed on Windows platform.
> On Feb. 6, 2018, 7:12 p.m., Andrew Schwartzmeyer wrote: > > Nit: Fix summary to be in past tense, and fix description to not be a copy > > of the summary. This was a one-line commit, but I went ahead and added a second line. I'm not sure why you said that the description shouldn't be a copy of the summary, it wasn't. Tooling problem on your side, or just a mistake? - Jeff --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65508/#review196911 --- On Feb. 5, 2018, 6:12 p.m., Jeff Coffler wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/65508/ > --- > > (Updated Feb. 5, 2018, 6:12 p.m.) > > > Review request for mesos, Andrew Schwartzmeyer and John Kordich. > > > Bugs: MESOS-6715 > https://issues.apache.org/jira/browse/MESOS-6715 > > > Repository: mesos > > > Description > --- > > Allow curl program to be properly executed on Windows platform. > > > Diffs > - > > src/uri/fetchers/curl.cpp f34daf2bc1f3fef74548e04ad332e3a627636bfd > > > Diff: https://reviews.apache.org/r/65508/diff/1/ > > > Testing > --- > > Full build, unit tests on both Linux and Windows platform. > > > Thanks, > > Jeff Coffler > >
Review Request 65510: Enable 'curl' uri_fetcher tests on Windows platform.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65510/ --- Review request for mesos, Andrew Schwartzmeyer and John Kordich. Bugs: MESOS-6715 https://issues.apache.org/jira/browse/MESOS-6715 Repository: mesos Description --- Enable 'curl' uri_fetcher tests on Windows platform. Diffs - src/tests/uri_fetcher_tests.cpp 8a40f46b215bb1f267a59a9edfca83445f86b430 Diff: https://reviews.apache.org/r/65510/diff/1/ Testing --- Thanks, Jeff Coffler
Review Request 65509: The CURL_ prefix in unit test name should always succeed on Windows.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65509/ --- Review request for mesos, Andrew Schwartzmeyer and John Kordich. Bugs: MESOS-6715 https://issues.apache.org/jira/browse/MESOS-6715 Repository: mesos Description --- Since the 'curl.exe' executable is built on part of Windows (it's a build target), it will always exist for test purposes. Diffs - src/tests/environment.cpp 13a4c9514fcd3016fe623c597decd067457e86cd Diff: https://reviews.apache.org/r/65509/diff/1/ Testing --- Thanks, Jeff Coffler
Review Request 65508: Allow curl program to be properly executed on Windows platform.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65508/ --- Review request for mesos, Andrew Schwartzmeyer and John Kordich. Bugs: MESOS-6715 https://issues.apache.org/jira/browse/MESOS-6715 Repository: mesos Description --- Allow curl program to be properly executed on Windows platform. Diffs - src/uri/fetchers/curl.cpp f34daf2bc1f3fef74548e04ad332e3a627636bfd Diff: https://reviews.apache.org/r/65508/diff/1/ Testing --- Full build, unit tests on both Linux and Windows platform. Thanks, Jeff Coffler
Re: Review Request 64962: Windows: Explicitly state source and destination path for extract.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64962/#review194908 --- Ship it! Ship It! - Jeff Coffler On Jan. 5, 2018, 9:50 p.m., Andrew Schwartzmeyer wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/64962/ > --- > > (Updated Jan. 5, 2018, 9:50 p.m.) > > > Review request for mesos, Akash Gupta, Jeff Coffler, Joseph Wu, and Michael > Park. > > > Repository: mesos > > > Description > --- > > Makes it more clear that it is in the correct order. > > > Diffs > - > > src/launcher/fetcher.cpp e2372a1b52d19937a62bd3fd5fe594c9e347e4a8 > > > Diff: https://reviews.apache.org/r/64962/diff/1/ > > > Testing > --- > > ctest -V on Windows (all passed) > > > Thanks, > > Andrew Schwartzmeyer > >
Re: Review Request 64962: Windows: Explicitly state source and destination path for extract.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64962/#review194907 --- src/launcher/fetcher.cpp Line 101 (original), 103 (patched) <https://reviews.apache.org/r/64962/#comment273990> I got the syntax straight from the help page from `help Expand-Archive`. But I guess if you don't know the parameters and don't want to get help, this will clarify. - Jeff Coffler On Jan. 5, 2018, 9:50 p.m., Andrew Schwartzmeyer wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/64962/ > --- > > (Updated Jan. 5, 2018, 9:50 p.m.) > > > Review request for mesos, Akash Gupta, Jeff Coffler, Joseph Wu, and Michael > Park. > > > Repository: mesos > > > Description > --- > > Makes it more clear that it is in the correct order. > > > Diffs > - > > src/launcher/fetcher.cpp e2372a1b52d19937a62bd3fd5fe594c9e347e4a8 > > > Diff: https://reviews.apache.org/r/64962/diff/1/ > > > Testing > --- > > ctest -V on Windows (all passed) > > > Thanks, > > Andrew Schwartzmeyer > >
Re: Review Request 64961: Windows: Fixed memory leak.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64961/#review194905 --- Ship it! Ship It! - Jeff Coffler On Jan. 5, 2018, 9:49 p.m., Andrew Schwartzmeyer wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/64961/ > --- > > (Updated Jan. 5, 2018, 9:49 p.m.) > > > Review request for mesos, Akash Gupta, Jeff Coffler, Joseph Wu, and Michael > Park. > > > Repository: mesos > > > Description > --- > > The environment strings variable block is allocated by the system, and > must be manually freed (according to the docs). > > > Diffs > - > > 3rdparty/stout/include/stout/os/windows/environment.hpp > f79cbfa17cbe47f024802f3be813759f8fd62baa > > > Diff: https://reviews.apache.org/r/64961/diff/1/ > > > Testing > --- > > ctest -V on Windows (all passed) > > > Thanks, > > Andrew Schwartzmeyer > >
Re: Review Request 64720: Upgraded to use cURL v7.57.0 on Windows Platform.
> On Dec. 19, 2017, 9:12 p.m., Mesos Reviewbot Windows wrote: > > FAIL: Cannot overwrite the item D:DCOSmesosbuild-outputbinariescurl.exe > > with itself. > > > > Reviews applied: `['64720']` > > > > All the build artifacts available at: > > http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/64720 I'm not 100% sure why reviewbot had this problem. I tested successfully on both Windows and Linux, and ran both STOUT and MESOS tests on both Windows on Linux prior to submitting the review. Note that my tests require changes that Andy made, so to get successful test passes on Windows (and to get a successful manual test), Andy's changes need to be in the chain. My changes require Andy's changes, which I indicated in "depends on". The logs (http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/64720/logs/mesos-tests-stdout.log) show that it built and passed all tests successfully. We will follow up with our CI team to fix this failure, but we can ignore it here. - Jeff --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64720/#review194179 ------- On Dec. 19, 2017, 7:33 p.m., Jeff Coffler wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/64720/ > --- > > (Updated Dec. 19, 2017, 7:33 p.m.) > > > Review request for mesos, Andrew Schwartzmeyer and John Kordich. > > > Repository: mesos > > > Description > --- > > Along with this, a number of changes were made: > > 1. Debug versions of cURL build with libcurl-d and curl-d.exe. >Modified build system to handle these issues. > 2. cURL is built to make use of SChannel (native Windows encryption) >by default. This means we no longer need to build Mesos with >cmake flag -DENABLE_SSL=ON, cURL isn't sensitive to that. > > > Diffs > - > > 3rdparty/CMakeLists.txt d9f52e20f2aa3bdc1544a9db70857aba02598439 > 3rdparty/cmake/Versions.cmake 13bd33ddc0dacc6dc8c4ac7e460e39fdb2c1327d > > > Diff: https://reviews.apache.org/r/64720/diff/1/ > > > Testing > --- > > > Thanks, > > Jeff Coffler > >
Review Request 64720: Upgraded to use cURL v7.57.0 on Windows Platform.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64720/ --- Review request for mesos, Andrew Schwartzmeyer and John Kordich. Repository: mesos Description --- Along with this, a number of changes were made: 1. Debug versions of cURL build with libcurl-d and curl-d.exe. Modified build system to handle these issues. 2. cURL is built to make use of SChannel (native Windows encryption) by default. This means we no longer need to build Mesos with cmake flag -DENABLE_SSL=ON, cURL isn't sensitive to that. Diffs - 3rdparty/CMakeLists.txt d9f52e20f2aa3bdc1544a9db70857aba02598439 3rdparty/cmake/Versions.cmake 13bd33ddc0dacc6dc8c4ac7e460e39fdb2c1327d Diff: https://reviews.apache.org/r/64720/diff/1/ Testing --- Thanks, Jeff Coffler
Re: Review Request 64689: Windows: Fixed `os::open()` to always use `O_BINARY`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64689/#review194167 --- Ship it! Ship It! - Jeff Coffler On Dec. 19, 2017, 3:34 a.m., Andrew Schwartzmeyer wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/64689/ > --- > > (Updated Dec. 19, 2017, 3:34 a.m.) > > > Review request for mesos, Akash Gupta, Jeff Coffler, Gaston Kleiman, and > Joseph Wu. > > > Repository: mesos > > > Description > --- > > Previously, we had been manually adding the `O_BINARY` flag as we > encountered bugs due to the Windows default behavior of performing > line-ending translation. This was error prone. > > Given this precedent, it seems safe to assume that all our existing code > expects the POSIX semantics of "binary mode", that is, no translation of > written data at all. So now we add this flag by default in `os::open()` > instead of in the users. > > It is possible that a future use requires text translation. At such > point, we can trivially fix `os::open()` to take a boolean flag to > control the addition of `O_BINARY`, but we do not currently need to > engineer this. > > > Diffs > - > > 3rdparty/stout/include/stout/net.hpp > 7b6557d40a83e1572bdc2dd89b5ff99fb8ed696a > 3rdparty/stout/include/stout/os/open.hpp > 1443b63d260b3da38073f234d15ffb4b97d4a736 > 3rdparty/stout/include/stout/os/write.hpp > 4c718b1a5055a742f16cac0bc5e88aa4ab6acfcd > 3rdparty/stout/include/stout/protobuf.hpp > baad12648dd78ab72ea4277f4c7f99da16696a40 > > > Diff: https://reviews.apache.org/r/64689/diff/2/ > > > Testing > --- > > > Thanks, > > Andrew Schwartzmeyer > >
Re: Review Request 64690: Windows: Removed manual use of `O_BINARY`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64690/#review194165 --- Ship it! Ship It! - Jeff Coffler On Dec. 19, 2017, 3:35 a.m., Andrew Schwartzmeyer wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/64690/ > --- > > (Updated Dec. 19, 2017, 3:35 a.m.) > > > Review request for mesos, Akash Gupta, Jeff Coffler, Gaston Kleiman, and > Joseph Wu. > > > Repository: mesos > > > Description > --- > > This is now superfluous as it is added in stout. > > > Diffs > - > > src/status_update_manager/status_update_manager_process.hpp > 5dc2bec14de972efa2dd00f9faf6c025f2e56e15 > > > Diff: https://reviews.apache.org/r/64690/diff/2/ > > > Testing > --- > > `ctest -V -C Debug` on Windows 10. > > > Thanks, > > Andrew Schwartzmeyer > >
Re: Review Request 63253: Fixed DefaultExecutorTest for proper URI handling on Windows.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63253/ --- (Updated Dec. 8, 2017, 2:46 a.m.) Review request for mesos, Andrew Schwartzmeyer, Gaston Kleiman, John Kordich, Joseph Wu, Li Li, and Michael Park. Bugs: MESOS-6705 https://issues.apache.org/jira/browse/MESOS-6705 Repository: mesos Description --- Fixed changes from Gaston (git commit 772c8f5 on 10/02/2017) for proper operation on Windows. Changed to use new URI methods introduced as part of MESOS-6705, and used powershell on Windows to replace "test" command on Linux. Diffs (updated) - src/tests/default_executor_tests.cpp d3e3ef770656a36f3469f2ec655102a05adcae8a Diff: https://reviews.apache.org/r/63253/diff/6/ Changes: https://reviews.apache.org/r/63253/diff/5-6/ Testing --- Ran mesos-tests on both Windows and Linux with no errors. Specifically ran mesos-tests with `--gtest_filter=MesosContainerizer/DefaultExecutorTest.*` to verify that the modified tests ran properly. Thanks, Jeff Coffler
Re: Review Request 60620: Modifed os::write to write binary files on Windows.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60620/ --- (Updated Dec. 8, 2017, 2:40 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 --- By default, ::write operations on Windows will include a \r\n (CR/LF) at the end of each line, which is not compatible with Linux. Mesos expects only \n (LF) characters between lines. This commit will modify Windows behavior to only include \n at the end of each line written to a text file. Diffs (updated) - 3rdparty/stout/include/stout/os/write.hpp 9ff749f209e6dd6ca3695907108a029c9a2b4f05 Diff: https://reviews.apache.org/r/60620/diff/8/ Changes: https://reviews.apache.org/r/60620/diff/7-8/ Testing --- Built successfully on both Linux (with autotools and cmake) and Windows (with cmake). Ran stout-tests and mesos-tests successfully on both Windows and Linux. Thanks, Jeff Coffler
Re: Review Request 60628: Enabled fetcher_tests.cpp unit test module on Windows platform.
> On Nov. 16, 2017, 11:08 p.m., Michael Park wrote:
> > src/tests/fetcher_tests.cpp
> > Line 469 (original), 473 (patched)
> > <https://reviews.apache.org/r/60628/diff/6/?file=1864359#file1864359line473>
> >
> > These would ideally be a `url::join(...)`, right?
> > I think for now we just use `strings::join('/', ...)`
> > in the codebase. Could we do that here and below, for now rather than
> > introducing a `url::join`?
>
> Jeff Coffler wrote:
> I guess, if we had a url::join(), but we don't, and I'm not really sure
> we need it (URLs always have `/` characters). I'm not 100% sure why you're
> suggesting strings::join, as that would just be more run-time work. The code
> is already building a string, and we're adding "/test" to it. Why do the work
> to build an extra substring ("/" + "test") and then add that, when we can
> just add the two strings together directly?
>
> Note that path::join was never correct, as we never wanted
> http://blah\test!
>
> Or are you suggesting `strings::join(http.process->self().id, "/test")`
> rather than `http.process->self().id + "/test")`?
>
> Jeff Coffler wrote:
> If you are suggesting `strings::join(http.process->self().id, "/test")`,
> is there a reason? Part of the coding standard I'm not aware of? Or you just
> prefer the syntax?
>
> Andrew Schwartzmeyer wrote:
> I think he's just suggesting:
>
> ```
> strings::join("/", http.process->self().id, "test");
> ```
>
> Like
> [this](https://github.com/apache/mesos/blob/02758c4e75483a0cd135fa465d1704d793bd4e48/3rdparty/stout/tests/strings_tests.cpp#L464).
>
> Like "join these strings with this separator".
Oh, I get it. Okay, no problem. Retested on both Windows and Linux, tests
passed for Fetcher. Will wait for all tests to complete, then will push latest
sources up for merge to master.
- Jeff
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60628/#review191264
---
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/60628/
> ---
>
> (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
> ---
>
> 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/6/
>
>
> Testing
> ---
>
> See upstream.
>
>
> Thanks,
>
> Jeff Coffler
>
>
Re: Review Request 60628: Enabled fetcher_tests.cpp unit test module on Windows platform.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60628/ --- (Updated Dec. 8, 2017, 2:45 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 --- 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 (updated) - src/launcher/CMakeLists.txt c7a83d476efe13d65fa529e7676b6488eb48f44b src/launcher/fetcher.cpp 5a9e93adbf2e4f0b8ff64a69e62fea5f6a0b8f2d src/slave/CMakeLists.txt 5782ddcd9e178b3b0790d121a17483056ae03d22 src/slave/containerizer/fetcher.cpp 89b73b739e1fee7ff2ff984bcaf44ade4f535411 src/tests/fetcher_tests.cpp df9d2d9586a6457004506c4e2a972ccfc912c7c5 Diff: https://reviews.apache.org/r/60628/diff/7/ Changes: https://reviews.apache.org/r/60628/diff/6-7/ Testing --- See upstream. Thanks, Jeff Coffler
Re: Review Request 60626: Eliminated os::shell calls from HDFS for Windows compatibility.
> On Nov. 16, 2017, 11:34 p.m., Jie Yu wrote: > > src/hdfs/hdfs.cpp > > Line 119 (original), 119 (patched) > > <https://reviews.apache.org/r/60626/diff/6/?file=1864354#file1864354line119> > > > > 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.
--- 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 60623: Converted "file://" URI handling to use new uri function.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60623/ --- (Updated Dec. 8, 2017, 2:43 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 --- Converted "file://" URI handling to use new uri function. Diffs (updated) - src/tests/credentials_tests.cpp 4ecb9778159e8eae3b017b83400d6a3168fa2476 src/tests/fetcher_tests.cpp df9d2d9586a6457004506c4e2a972ccfc912c7c5 src/tests/master_contender_detector_tests.cpp f499136c0b981072af5bc8accf2238b7ba4bf430 src/tests/script.cpp 9823e334896895cfea8ccf9faf44529050f0f1c3 Diff: https://reviews.apache.org/r/60623/diff/7/ Changes: https://reviews.apache.org/r/60623/diff/6-7/ Testing --- See upstream Thanks, Jeff Coffler
Re: Review Request 60624: Enabled HDFS compilation and associated tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60624/ --- (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 --- Note that HDFS tests are disabled for Windows due to dependence on 'sh' shell. Be aware: HDFS hasn't been tested, although it should work. We will formally add support for HDFS at a later date, see MESOS-5460. Diffs (updated) - src/CMakeLists.txt 35a602d2afb3a1e6ef76a0b0df2628ce5493dc81 src/tests/CMakeLists.txt 92db731a81303f0d1d95dfe0b80a0a512e165445 src/tests/hdfs_tests.cpp e7154c75e663d9a98bec48be42a59b65c96f9515 Diff: https://reviews.apache.org/r/60624/diff/7/ Changes: https://reviews.apache.org/r/60624/diff/6-7/ Testing --- See upstream Thanks, Jeff Coffler
Re: Review Request 60622: Added new stout functions for path and URI conversions.
> On Nov. 16, 2017, 6:08 p.m., Michael Park wrote: > > 3rdparty/stout/include/stout/uri.hpp > > Lines 19 (patched) > > <https://reviews.apache.org/r/60622/diff/6/?file=1864343#file1864343line19> > > > > This looks like an accidental include? Yup, good catch, thanks. > On Nov. 16, 2017, 6:08 p.m., Michael Park wrote: > > 3rdparty/stout/include/stout/uri.hpp > > Lines 31 (patched) > > <https://reviews.apache.org/r/60622/diff/6/?file=1864343#file1864343line31> > > > > This should be `#ifdef __WINDOWS__`, right? Yup. And line 34 should have included `// __WINDOWS__` at the end. - Jeff --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60622/#review191203 ------- On Nov. 6, 2017, 6:08 p.m., Jeff Coffler wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60622/ > --- > > (Updated Nov. 6, 2017, 6:08 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 > --- > > Added new stout function: path::from_uri, which converts to a filepath > from a URI. > > Also added uri::from_path, which generates a URI from a file path. > > > Diffs > - > > 3rdparty/stout/Makefile.am 4386017acd6ca465be3f735460c11d50b440ccc5 > 3rdparty/stout/include/Makefile.am bdd3e9908ebfc682458a3babc34cbee36ad3f751 > 3rdparty/stout/include/stout/path.hpp > 6ee3a44cd6a878fe383aa68df40b82857b93d0b4 > 3rdparty/stout/include/stout/uri.hpp PRE-CREATION > 3rdparty/stout/tests/CMakeLists.txt > 6e5773f1e03671de7ac007caf49edd0f1cd7aedd > 3rdparty/stout/tests/path_tests.cpp > f8c14d5aefe0b49adb778da784143a328c96183d > 3rdparty/stout/tests/uri_tests.cpp PRE-CREATION > > > Diff: https://reviews.apache.org/r/60622/diff/6/ > > > Testing > --- > > See upstream > > > Thanks, > > Jeff Coffler > >
Re: Review Request 60622: Added new stout functions for path and URI conversions.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60622/ --- (Updated Dec. 8, 2017, 2:43 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 --- Added new stout function: path::from_uri, which converts to a filepath from a URI. Also added uri::from_path, which generates a URI from a file path. Diffs (updated) - 3rdparty/stout/Makefile.am b1318847df486f9f96c8b005998310bb56af91a9 3rdparty/stout/include/Makefile.am 4ba91bbedecbe7914cf9f5c6a815b60ca02f0e01 3rdparty/stout/include/stout/path.hpp 6ee3a44cd6a878fe383aa68df40b82857b93d0b4 3rdparty/stout/include/stout/uri.hpp PRE-CREATION 3rdparty/stout/tests/CMakeLists.txt bc2498d6fca1161c42172f5bd09a32785e0a2128 3rdparty/stout/tests/path_tests.cpp f8c14d5aefe0b49adb778da784143a328c96183d 3rdparty/stout/tests/uri_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/60622/diff/7/ Changes: https://reviews.apache.org/r/60622/diff/6-7/ Testing --- See upstream Thanks, Jeff Coffler
Re: Review Request 60621: Added new stout capability: os::copyfile(source, dest).
> On Nov. 16, 2017, 5:50 p.m., Michael Park wrote: > > 3rdparty/stout/include/stout/os/posix/copyfile.hpp > > Lines 45 (patched) > > <https://reviews.apache.org/r/60621/diff/6/?file=1864336#file1864336line45> > > > > The backslash at the end check seems a bit odd. Doesn't `isdir` return > > `true` if `/foo/bar/` is a directory? We also wouldn't be catching other > > cases like `/foo/bar/.` here, right? isdir will return true, but it's a stat:: operation, so it will only return true if a directory existed. The goal here was to try to limit Posix operations to be as follows: 1. Limits should not interfere with what Mesos was already doing, and 2. Limits should roughly put same restrictions in place as the Windows API code did. This probably wasn't perfect, but I felt it was a good stab, and generally good enough. No, I think you're right, we wouldn't catch all cases (like /foo/bar/.), but I wasn't after every single case. Just to do basic limits. The crux of the problem here is that, on Posix, we use 'cp'. If you think about 'cp' behavior, it's pretty wierd from an API perspective (you can't determine what it will do without knowing state of things). I was trying to make it more deterministic, but I never intended to be "perfect" about it. > On Nov. 16, 2017, 5:50 p.m., Michael Park wrote: > > 3rdparty/stout/include/stout/os/posix/copyfile.hpp > > Lines 65 (patched) > > <https://reviews.apache.org/r/60621/diff/6/?file=1864336#file1864336line65> > > > > I believe this is equivalent to > > ``` > > if (!WSUCCEEDED(status)) > > ``` Why, yes, it is. The problem is that WSUCCEEDED is defined in src/common/status_utils.hpp, and this is stout, so I can't include that here. - Jeff --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60621/#review191201 --- On Nov. 6, 2017, 6:08 p.m., Jeff Coffler wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60621/ > --- > > (Updated Nov. 6, 2017, 6:08 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 > --- > > The os::copyfile() method provides an API to copy a file from one > location to another. Note that copying of directories are not > supported, and that the destination directory must exist before a > file can be copied into a directory. > > > Diffs > - > > 3rdparty/stout/Makefile.am 4386017acd6ca465be3f735460c11d50b440ccc5 > 3rdparty/stout/include/Makefile.am bdd3e9908ebfc682458a3babc34cbee36ad3f751 > 3rdparty/stout/include/stout/os/copyfile.hpp PRE-CREATION > 3rdparty/stout/include/stout/os/posix/copyfile.hpp PRE-CREATION > 3rdparty/stout/include/stout/os/windows/copyfile.hpp PRE-CREATION > 3rdparty/stout/tests/CMakeLists.txt > 6e5773f1e03671de7ac007caf49edd0f1cd7aedd > 3rdparty/stout/tests/os/copyfile_tests.cpp PRE-CREATION > > > Diff: https://reviews.apache.org/r/60621/diff/6/ > > > Testing > --- > > See upstream > > Note that Joe made some changes to this, I ended up taking his changes as is. > > > Thanks, > > Jeff Coffler > >
Re: Review Request 60621: Added new stout capability: os::copyfile(source, dest).
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60621/ --- (Updated Dec. 8, 2017, 2:41 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 --- The os::copyfile() method provides an API to copy a file from one location to another. Note that copying of directories are not supported, and that the destination directory must exist before a file can be copied into a directory. Diffs (updated) - 3rdparty/stout/Makefile.am b1318847df486f9f96c8b005998310bb56af91a9 3rdparty/stout/include/Makefile.am 4ba91bbedecbe7914cf9f5c6a815b60ca02f0e01 3rdparty/stout/include/stout/os/copyfile.hpp PRE-CREATION 3rdparty/stout/include/stout/os/posix/copyfile.hpp PRE-CREATION 3rdparty/stout/include/stout/os/windows/copyfile.hpp PRE-CREATION 3rdparty/stout/tests/CMakeLists.txt bc2498d6fca1161c42172f5bd09a32785e0a2128 3rdparty/stout/tests/os/copyfile_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/60621/diff/7/ Changes: https://reviews.apache.org/r/60621/diff/6-7/ Testing --- See upstream Note that Joe made some changes to this, I ended up taking his changes as is. Thanks, Jeff Coffler
Re: Review Request 60628: Enabled fetcher_tests.cpp unit test module on Windows platform.
> On Nov. 16, 2017, 11:08 p.m., Michael Park wrote:
> > src/tests/fetcher_tests.cpp
> > Line 469 (original), 473 (patched)
> > <https://reviews.apache.org/r/60628/diff/6/?file=1864359#file1864359line473>
> >
> > These would ideally be a `url::join(...)`, right?
> > I think for now we just use `strings::join('/', ...)`
> > in the codebase. Could we do that here and below, for now rather than
> > introducing a `url::join`?
>
> Jeff Coffler wrote:
> I guess, if we had a url::join(), but we don't, and I'm not really sure
> we need it (URLs always have `/` characters). I'm not 100% sure why you're
> suggesting strings::join, as that would just be more run-time work. The code
> is already building a string, and we're adding "/test" to it. Why do the work
> to build an extra substring ("/" + "test") and then add that, when we can
> just add the two strings together directly?
>
> Note that path::join was never correct, as we never wanted
> http://blah\test!
>
> Or are you suggesting `strings::join(http.process->self().id, "/test")`
> rather than `http.process->self().id + "/test")`?
If you are suggesting `strings::join(http.process->self().id, "/test")`, is
there a reason? Part of the coding standard I'm not aware of? Or you just
prefer the syntax?
- Jeff
-------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60628/#review191264
---
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/60628/
> ---
>
> (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
> ---
>
> 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/6/
>
>
> Testing
> ---
>
> See upstream.
>
>
> Thanks,
>
> Jeff Coffler
>
>
Re: Review Request 60628: Enabled fetcher_tests.cpp unit test module on Windows platform.
> On Nov. 16, 2017, 11:08 p.m., Michael Park wrote:
> > src/tests/fetcher_tests.cpp
> > Line 469 (original), 473 (patched)
> > <https://reviews.apache.org/r/60628/diff/6/?file=1864359#file1864359line473>
> >
> > These would ideally be a `url::join(...)`, right?
> > I think for now we just use `strings::join('/', ...)`
> > in the codebase. Could we do that here and below, for now rather than
> > introducing a `url::join`?
I guess, if we had a url::join(), but we don't, and I'm not really sure we need
it (URLs always have `/` characters). I'm not 100% sure why you're suggesting
strings::join, as that would just be more run-time work. The code is already
building a string, and we're adding "/test" to it. Why do the work to build an
extra substring ("/" + "test") and then add that, when we can just add the two
strings together directly?
Note that path::join was never correct, as we never wanted http://blah\test!
Or are you suggesting `strings::join(http.process->self().id, "/test")` rather
than `http.process->self().id + "/test")`?
- Jeff
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60628/#review191264
---
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/60628/
> ---
>
> (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
> ---
>
> 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/6/
>
>
> Testing
> ---
>
> See upstream.
>
>
> Thanks,
>
> Jeff Coffler
>
>
Re: Review Request 60621: Added new stout capability: os::copyfile(source, dest).
> On Nov. 3, 2017, 6:29 p.m., Benjamin Mahler wrote:
> > 3rdparty/stout/include/stout/os/windows/copyfile.hpp
> > Lines 56-57 (patched)
> > <https://reviews.apache.org/r/60621/diff/6/?file=1864337#file1864337line56>
> >
> > The general pattern is to just include the reason for an error, and to
> > not include any information the caller already has, because otherwise the
> > callers will double log:
> >
> > ```
> > Try copy = copyfile(source, destination);
> >
> > if (copy.isError()) {
> > return ("Failed to copy '" + source + "'"
> > " to '" + destination + "': " + copy.error();
> > }
> > ```
> >
> > The current code would log:
> >
> > "Failed to copy 's' to 'd': Failed to copy 's' to 'd': No disk space
> > left"
> >
> > Can you guys do a sweep for this issue in the windows related code that
> > has been added?
>
> Andrew Schwartzmeyer wrote:
> I'm not sure I follow. Are you saying the _caller_ should always write
> the actual error message, versus the _callee_ preparing it here?
>
> I guess in your example, I don't get why the user of `os::copyfile` would
> add `"Failed to copy..."` instead of just doing:
>
> ```
> if (copy.isError()) {
> return Error(copy.error());
> }
> ```
>
> And then the error message is only written once, in `os::copyfile`.
>
> Benjamin Mahler wrote:
> What is the "actual" error message? :)
>
> An error message consists of several parts, much like an exception: the
> "reason" for the error, and multiple "stacks" of context. If you're referring
> to the "reason" when you said "actual", either approach (the one we use, or
> the one used in this patch) includes the reason in their returned error
> message. The distinction lies in where the "stacks" of context get included.
>
> The decision we took some time ago was to have the "owner" of the context
> be responsible for including it. So if I call `os::copyfile` I know which
> function I'm calling and which source and destination I'm passing into it.
> This matches posix-style programming which I why I think we chose this
> approach:
>
> ```
> int main()
> {
> int fd = open("/file");
>
> if (fd == -1) {
> // Caller logs the thing it was doing, and gets the reason for the
> error:
> LOG(ERROR) << "Failed to initialize: Failed to open '/file': " <<
> strerror(errno);
> }
> }
>
> vs
>
> int main()
> {
> Try fd = open("/file");
>
> if (fd.isError()) {
> // Caller logs the thing it was doing, and gets the reason for the
> error:
> LOG(ERROR) << "Failed to initialize: Failed to open '/file': " <<
> fd.error();
> }
> }
> ```
>
> Now of course, if you use the alternative approach to have the leaf
> include all the information it has, then you have to compose differently:
>
> ```
> int main()
> {
> Try fd = os::open("/file");
>
> if (fd.isError()) {
> // Caller knows that no additional context needs to be added because
> callee has all of it.
> LOG(ERROR) << "Failed to initialize: " << fd.error();
> }
> }
> ```
>
> The approach we chose was to treat the error as just the "reason" (like
> strerror), so if you want to add context to it you can.
>
> Both approaches work, but we have to pick one and apply it consistently
> as best we can. In retrospect, I actually think the other approach would have
> been a better choice because it fits more easily into Future::then style
> composition. But we would have to discuss the change of approach and do a
> sweep, because most of the code follows the first pattern shown.
>
> Andrew Schwartzmeyer wrote:
> Ah, thank you for the detailed explanation. Unfortunately, the existing
> error handling code was using the leaf approach, and we kept it consistent,
> but consistently wrong. We'll have to do a sweep to fix it.
>
> This explanation was really good, do we have something like it
> docume
Re: Review Request 63253: Fixed DefaultExecutorTest for proper URI handling on Windows.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63253/ --- (Updated Nov. 8, 2017, 12:37 a.m.) Review request for mesos, Andrew Schwartzmeyer, Gaston Kleiman, John Kordich, Joseph Wu, Li Li, and Michael Park. Bugs: MESOS-6705 https://issues.apache.org/jira/browse/MESOS-6705 Repository: mesos Description --- Fixed changes from Gaston (git commit 772c8f5 on 10/02/2017) for proper operation on Windows. Changed to use new URI methods introduced as part of MESOS-6705, and used powershell on Windows to replace "test" command on Linux. Diffs (updated) - src/tests/default_executor_tests.cpp 5078bd4d70698f5cbd14c971fcecfd58f8467a04 Diff: https://reviews.apache.org/r/63253/diff/5/ Changes: https://reviews.apache.org/r/63253/diff/4-5/ Testing --- Ran mesos-tests on both Windows and Linux with no errors. Specifically ran mesos-tests with `--gtest_filter=MesosContainerizer/DefaultExecutorTest.*` to verify that the modified tests ran properly. Thanks, Jeff Coffler
Re: Review Request 63272: Windows: Added `os::get_job_info` to stout.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63272/#review189740 --- Ship it! Ship It! - Jeff Coffler On Oct. 31, 2017, 5:51 p.m., Andrew Schwartzmeyer wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/63272/ > --- > > (Updated Oct. 31, 2017, 5:51 p.m.) > > > Review request for mesos, Akash Gupta, Jeff Coffler, Jie Yu, John Kordich, > and Joseph Wu. > > > Repository: mesos > > > Description > --- > > This returns a `JOBOBJECT_BASIC_ACCOUNTING_INFORMATION`, which can be > inspected for basic CPU / process accounting information for a group of > processes within a job object. > > > Diffs > - > > 3rdparty/stout/include/stout/windows/os.hpp > 09ddec6d69472cd13b453fe1a77fdbe343fc23c8 > > > Diff: https://reviews.apache.org/r/63272/diff/2/ > > > Testing > --- > > > Thanks, > > Andrew Schwartzmeyer > >
Re: Review Request 63279: Increased check tests task resources for Windows.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63279/#review189363 --- src/tests/check_tests.cpp Line 80 (original), 80 (patched) <https://reviews.apache.org/r/63279/#comment266395> Nit: Please fix commit message: `Realistically, the safe mimimum is 512 MB of.` Of what? (Memory, obviously, but please fix.) - Jeff Coffler On Oct. 26, 2017, 4:40 p.m., Andrew Schwartzmeyer wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/63279/ > --- > > (Updated Oct. 26, 2017, 4:40 p.m.) > > > Review request for mesos, Akash Gupta, Alexander Rukletsov, Jeff Coffler, > Gaston Kleiman, Jie Yu, John Kordich, Joseph Wu, and Li Li. > > > Bugs: MESOS-6690 > https://issues.apache.org/jira/browse/MESOS-6690 > > > Repository: mesos > > > Description > --- > > Unfortunately, due to PowerShell's resource usage as a .NET program, on > Windows 32 MB is not enough memory to run these tests. One instance of > PowerShell takes > 128 MB, and two instances take > 256 MB. > Realistically, the safe minimum is 512 MB of. > > > Diffs > - > > src/tests/check_tests.cpp d3ffc0b0204d021f1ed734ece189d0e3a3fd8844 > > > Diff: https://reviews.apache.org/r/63279/diff/1/ > > > Testing > --- > > Built and ran `mesos-tests.exe` on Windows repeatedly, verified all tests > still pass consistently (likewise for `stout` and `libprocess` tests. > > ~~Currently verifying no breaks on Linux.~~ Built and ran tests on Linux. > > NOTE: There are more check tests that are currently disabled for Windows, > that I think should be enabled, but did immediately work for me, so I've left > them disabled to unblock myself. Similarly, I would ideally like to port the > balloon example framework, and use that to prove the effectiveness of the job > object memory hard-cap. Having not yet ported it though, I manually verified > the effectiveness of the new isolators by launching test CPU and memory test > tasks on a deployed cluster my these changes (and the fact that PowerShell > OOM'ed was a nice verification too). > > > Thanks, > > Andrew Schwartzmeyer > >
Re: Review Request 63279: Increased check tests task resources for Windows.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63279/#review189362 --- Ship it! Ship It! - Jeff Coffler On Oct. 26, 2017, 4:40 p.m., Andrew Schwartzmeyer wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/63279/ > --- > > (Updated Oct. 26, 2017, 4:40 p.m.) > > > Review request for mesos, Akash Gupta, Alexander Rukletsov, Jeff Coffler, > Gaston Kleiman, Jie Yu, John Kordich, Joseph Wu, and Li Li. > > > Bugs: MESOS-6690 > https://issues.apache.org/jira/browse/MESOS-6690 > > > Repository: mesos > > > Description > --- > > Unfortunately, due to PowerShell's resource usage as a .NET program, on > Windows 32 MB is not enough memory to run these tests. One instance of > PowerShell takes > 128 MB, and two instances take > 256 MB. > Realistically, the safe minimum is 512 MB of. > > > Diffs > - > > src/tests/check_tests.cpp d3ffc0b0204d021f1ed734ece189d0e3a3fd8844 > > > Diff: https://reviews.apache.org/r/63279/diff/1/ > > > Testing > --- > > Built and ran `mesos-tests.exe` on Windows repeatedly, verified all tests > still pass consistently (likewise for `stout` and `libprocess` tests. > > ~~Currently verifying no breaks on Linux.~~ Built and ran tests on Linux. > > NOTE: There are more check tests that are currently disabled for Windows, > that I think should be enabled, but did immediately work for me, so I've left > them disabled to unblock myself. Similarly, I would ideally like to port the > balloon example framework, and use that to prove the effectiveness of the job > object memory hard-cap. Having not yet ported it though, I manually verified > the effectiveness of the new isolators by launching test CPU and memory test > tasks on a deployed cluster my these changes (and the fact that PowerShell > OOM'ed was a nice verification too). > > > Thanks, > > Andrew Schwartzmeyer > >
Re: Review Request 63278: Windows: Documented the `cpuset` and `mem` isolators.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63278/#review189360 --- Ship it! Ship It! - Jeff Coffler On Oct. 26, 2017, 4:34 p.m., Andrew Schwartzmeyer wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/63278/ > --- > > (Updated Oct. 26, 2017, 4:34 p.m.) > > > Review request for mesos, Akash Gupta, Jeff Coffler, Jie Yu, John Kordich, > Joseph Wu, and Li Li. > > > Bugs: MESOS-6690 > https://issues.apache.org/jira/browse/MESOS-6690 > > > Repository: mesos > > > Description > --- > > This adds documentation on the usage of the job object isolators, which > enable task limits, as well as the statistics they report. > > > Diffs > - > > docs/isolators/windows.md PRE-CREATION > docs/mesos-containerizer.md 9cb071273a5ce42784457ea01b363911fdb7773d > > > Diff: https://reviews.apache.org/r/63278/diff/1/ > > > Testing > --- > > > Thanks, > > Andrew Schwartzmeyer > >
Re: Review Request 63277: Windows: Ported CPU and memory isolator tests.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63277/#review189358
---
src/tests/containerizer/cpu_isolator_tests.cpp
Lines 120 (patched)
<https://reviews.apache.org/r/63277/#comment266383>
Given what we learned about quoting, why not just do:
"powershell -c while ($true) {}"
?
src/tests/containerizer/cpu_isolator_tests.cpp
Line 163 (original), 172 (patched)
<https://reviews.apache.org/r/63277/#comment266386>
A powershell loop wouldn't do it. `Get-Process` is a step in the right
direction, but probably has a small amount of kernel time and a lot of
PowerShell time for formatting, etc.
What about some sort of tight loop that was essentially exclusively a
kernel operation, like mapping and unmapping a memory segment in a loop or
something? This could be done in C, avoids PowerShell entirely, and would need
to be processed by the kernel. Or creating a shared memory mutex, or anything
along those lines.
The kernel is supposed to be efficient, so it would be hard to consume
Kernel time, but pick something that you know is 100% kernel, and do it in a
tight loop. That should get you there.
src/tests/containerizer/cpu_isolator_tests.cpp
Lines 224 (patched)
<https://reviews.apache.org/r/63277/#comment266385>
Ditto
src/tests/containerizer/cpu_isolator_tests.cpp
Line 234 (original), 253 (patched)
<https://reviews.apache.org/r/63277/#comment266387>
If you're doing a `sleep` operation, that won't consume system time. The
kernel will deschedule you until it's time for you to wake up.
You might get there by doing a lot of short sleeps, but that's generally
risky and unreliable (depends on kernel timing, speed of implementation, etc).
- Jeff Coffler
On Oct. 26, 2017, 4:33 p.m., Andrew Schwartzmeyer wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63277/
> ---
>
> (Updated Oct. 26, 2017, 4:33 p.m.)
>
>
> Review request for mesos, Akash Gupta, Jeff Coffler, Jie Yu, John Kordich,
> Joseph Wu, and Li Li.
>
>
> Bugs: MESOS-6690
> https://issues.apache.org/jira/browse/MESOS-6690
>
>
> Repository: mesos
>
>
> Description
> ---
>
> These test the limitation and usage reporting of the new Windows
> `Cpuset` and `Mem` isolators.
>
>
> Diffs
> -
>
> src/tests/CMakeLists.txt 386e0473c93d0a993248c7818067071d0c761c76
> src/tests/containerizer/cpu_isolator_tests.cpp
> 846b2e255547a02f5eb0590747edca62bc560ac3
> src/tests/containerizer/memory_isolator_tests.cpp
> b8ea5d35b3a0a4820d9ec4c6d7d916dc6101b570
> src/tests/mesos.cpp 9185b5bf2175be5b0f6b6a03a04e9e5445bf22fd
>
>
> Diff: https://reviews.apache.org/r/63277/diff/1/
>
>
> Testing
> ---
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 63275: Windows: Abstracted out `os::name_job` in stout.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63275/#review189355 --- Ship it! Ship It! - Jeff Coffler On Oct. 26, 2017, 4:31 p.m., Andrew Schwartzmeyer wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/63275/ > --- > > (Updated Oct. 26, 2017, 4:31 p.m.) > > > Review request for mesos, Akash Gupta, Jeff Coffler, Jie Yu, John Kordich, > and Joseph Wu. > > > Repository: mesos > > > Description > --- > > Almost all uses identify a job object by the PID, but the job object > handle must be obtained by name. So this overloads `os::open_job` to > call `os::name_job` when given a PID. > > > Diffs > - > > 3rdparty/stout/include/stout/windows/os.hpp > 09ddec6d69472cd13b453fe1a77fdbe343fc23c8 > > > Diff: https://reviews.apache.org/r/63275/diff/1/ > > > Testing > --- > > > Thanks, > > Andrew Schwartzmeyer > >
Re: Review Request 63271: Windows: Added `os::set_job_memory_limit` to stout.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63271/#review189347 --- Ship it! Ship It! - Jeff Coffler On Oct. 26, 2017, 4:29 p.m., Andrew Schwartzmeyer wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/63271/ > --- > > (Updated Oct. 26, 2017, 4:29 p.m.) > > > Review request for mesos, Akash Gupta, Jeff Coffler, Jie Yu, John Kordich, > and Joseph Wu. > > > Repository: mesos > > > Description > --- > > This is used to set a hard cap on the memory usage for a job object. > > > Diffs > - > > 3rdparty/stout/include/stout/windows/os.hpp > 09ddec6d69472cd13b453fe1a77fdbe343fc23c8 > > > Diff: https://reviews.apache.org/r/63271/diff/1/ > > > Testing > --- > > > Thanks, > > Andrew Schwartzmeyer > >
Re: Review Request 63273: Windows: Added `os::get_job_processes` to stout.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63273/#review189346 --- Ship it! Ship It! - Jeff Coffler On Oct. 26, 2017, 4:31 p.m., Andrew Schwartzmeyer wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/63273/ > --- > > (Updated Oct. 26, 2017, 4:31 p.m.) > > > Review request for mesos, Akash Gupta, Jeff Coffler, Jie Yu, John Kordich, > Joseph Wu, and Michael Park. > > > Repository: mesos > > > Description > --- > > Returns a `set` for all the processes in a job object. > > > Diffs > - > > 3rdparty/stout/include/stout/windows/os.hpp > 09ddec6d69472cd13b453fe1a77fdbe343fc23c8 > > > Diff: https://reviews.apache.org/r/63273/diff/1/ > > > Testing > --- > > > Thanks, > > Andrew Schwartzmeyer > >
Re: Review Request 63272: Windows: Added `os::get_job_info` to stout.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63272/#review189343 --- 3rdparty/stout/include/stout/windows/os.hpp Lines 730 (patched) <https://reviews.apache.org/r/63272/#comment266362> This line is > 80 bytes, needs `NOLINT` added. - Jeff Coffler On Oct. 26, 2017, 4:30 p.m., Andrew Schwartzmeyer wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/63272/ > --- > > (Updated Oct. 26, 2017, 4:30 p.m.) > > > Review request for mesos, Akash Gupta, Jeff Coffler, Jie Yu, John Kordich, > and Joseph Wu. > > > Repository: mesos > > > Description > --- > > This returns a `JOBOBJECT_BASIC_ACCOUNTING_INFORMATION`, which can be > inspected for basic CPU / process accounting information for a group of > processes within a job object. > > > Diffs > - > > 3rdparty/stout/include/stout/windows/os.hpp > 09ddec6d69472cd13b453fe1a77fdbe343fc23c8 > > > Diff: https://reviews.apache.org/r/63272/diff/1/ > > > Testing > --- > > > Thanks, > > Andrew Schwartzmeyer > >
Re: Review Request 63271: Windows: Added `os::set_job_memory_limit` to stout.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63271/#review189342 --- 3rdparty/stout/include/stout/windows/os.hpp Lines 784 (patched) <https://reviews.apache.org/r/63271/#comment266361> I usually see NOLINT on a line of it's own. Is that a mistake, making the line even longer with `NOLINT`? - Jeff Coffler On Oct. 26, 2017, 4:29 p.m., Andrew Schwartzmeyer wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/63271/ > --- > > (Updated Oct. 26, 2017, 4:29 p.m.) > > > Review request for mesos, Akash Gupta, Jeff Coffler, Jie Yu, John Kordich, > and Joseph Wu. > > > Repository: mesos > > > Description > --- > > This is used to set a hard cap on the memory usage for a job object. > > > Diffs > - > > 3rdparty/stout/include/stout/windows/os.hpp > 09ddec6d69472cd13b453fe1a77fdbe343fc23c8 > > > Diff: https://reviews.apache.org/r/63271/diff/1/ > > > Testing > --- > > > Thanks, > > Andrew Schwartzmeyer > >
Re: Review Request 63268: Windows: Fixed variable casing in `windows/os.hpp`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63268/#review189340 --- Ship it! Ship It! - Jeff Coffler On Oct. 26, 2017, 4:27 p.m., Andrew Schwartzmeyer wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/63268/ > --- > > (Updated Oct. 26, 2017, 4:27 p.m.) > > > Review request for mesos, Akash Gupta, Jeff Coffler, Jie Yu, John Kordich, > and Joseph Wu. > > > Repository: mesos > > > Description > --- > > This changes the CamelCase to snake_case per the style guide for stout. > It also adds `::` to uses of `::GetLastError` where it was missing, and > includes `` because it was used but missing. > > > Diffs > - > > 3rdparty/stout/include/stout/windows/os.hpp > 09ddec6d69472cd13b453fe1a77fdbe343fc23c8 > > > Diff: https://reviews.apache.org/r/63268/diff/1/ > > > Testing > --- > > > Thanks, > > Andrew Schwartzmeyer > >
Re: Review Request 63253: Fixed DefaultExecutorTest for proper URI handling on Windows.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63253/ --- (Updated Oct. 25, 2017, 11:19 p.m.) Review request for mesos, Andrew Schwartzmeyer, Gaston Kleiman, John Kordich, Joseph Wu, and Li Li. Bugs: MESOS-6705 https://issues.apache.org/jira/browse/MESOS-6705 Repository: mesos Description --- Fixed changes from Gaston (git commit 772c8f5 on 10/02/2017) for proper operation on Windows. Changed to use new URI methods introduced as part of MESOS-6705, and used powershell on Windows to replace "test" command on Linux. Diffs (updated) - src/tests/default_executor_tests.cpp 5078bd4d70698f5cbd14c971fcecfd58f8467a04 Diff: https://reviews.apache.org/r/63253/diff/4/ Changes: https://reviews.apache.org/r/63253/diff/3-4/ Testing --- Ran mesos-tests on both Windows and Linux with no errors. Specifically ran mesos-tests with `--gtest_filter=MesosContainerizer/DefaultExecutorTest.*` to verify that the modified tests ran properly. Thanks, Jeff Coffler
Re: Review Request 63253: Fixed DefaultExecutorTest for proper URI handling on Windows.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63253/ --- (Updated Oct. 25, 2017, 10:17 p.m.) Review request for mesos, Andrew Schwartzmeyer, Gaston Kleiman, John Kordich, Joseph Wu, and Li Li. Bugs: MESOS-6705 https://issues.apache.org/jira/browse/MESOS-6705 Repository: mesos Description --- Fixed changes from Gaston (git commit 772c8f5 on 10/02/2017) for proper operation on Windows. Changed to use new URI methods introduced as part of MESOS-6705, and used powershell on Windows to replace "test" command on Linux. Diffs (updated) - src/tests/default_executor_tests.cpp 5078bd4d70698f5cbd14c971fcecfd58f8467a04 Diff: https://reviews.apache.org/r/63253/diff/3/ Changes: https://reviews.apache.org/r/63253/diff/2-3/ Testing --- Ran mesos-tests on both Windows and Linux with no errors. Specifically ran mesos-tests with `--gtest_filter=MesosContainerizer/DefaultExecutorTest.*` to verify that the modified tests ran properly. Thanks, Jeff Coffler
Re: Review Request 63253: Fixed DefaultExecutorTest for proper URI handling on Windows.
> On Oct. 25, 2017, 10:07 p.m., Gaston Kleiman wrote: > > src/tests/default_executor_tests.cpp > > Lines 1657 (patched) > > <https://reviews.apache.org/r/63253/diff/2/?file=1868878#file1868878line1660> > > > > Nit: do we need this the whitespace after the opening quotes? There used to be a line before that (to invoke powershell), but it's gone now. Space removed after the opening quotes, and removed one space from the line below. - Jeff --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63253/#review189252 ------- On Oct. 25, 2017, 7:59 p.m., Jeff Coffler wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/63253/ > --- > > (Updated Oct. 25, 2017, 7:59 p.m.) > > > Review request for mesos, Andrew Schwartzmeyer, Gaston Kleiman, John Kordich, > Joseph Wu, and Li Li. > > > Bugs: MESOS-6705 > https://issues.apache.org/jira/browse/MESOS-6705 > > > Repository: mesos > > > Description > --- > > Fixed changes from Gaston (git commit 772c8f5 on 10/02/2017) > for proper operation on Windows. Changed to use new URI methods > introduced as part of MESOS-6705, and used powershell on Windows > to replace "test" command on Linux. > > > Diffs > - > > src/tests/default_executor_tests.cpp > 5078bd4d70698f5cbd14c971fcecfd58f8467a04 > > > Diff: https://reviews.apache.org/r/63253/diff/2/ > > > Testing > --- > > Ran mesos-tests on both Windows and Linux with no errors. > > Specifically ran mesos-tests with > `--gtest_filter=MesosContainerizer/DefaultExecutorTest.*` to verify that the > modified tests ran properly. > > > Thanks, > > Jeff Coffler > >
Re: Review Request 63253: Fixed DefaultExecutorTest for proper URI handling on Windows.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63253/ --- (Updated Oct. 25, 2017, 7:59 p.m.) Review request for mesos, Andrew Schwartzmeyer, Gaston Kleiman, John Kordich, Joseph Wu, and Li Li. Bugs: MESOS-6705 https://issues.apache.org/jira/browse/MESOS-6705 Repository: mesos Description --- Fixed changes from Gaston (git commit 772c8f5 on 10/02/2017) for proper operation on Windows. Changed to use new URI methods introduced as part of MESOS-6705, and used powershell on Windows to replace "test" command on Linux. Diffs (updated) - src/tests/default_executor_tests.cpp 5078bd4d70698f5cbd14c971fcecfd58f8467a04 Diff: https://reviews.apache.org/r/63253/diff/2/ Changes: https://reviews.apache.org/r/63253/diff/1-2/ Testing --- Ran mesos-tests on both Windows and Linux with no errors. Specifically ran mesos-tests with `--gtest_filter=MesosContainerizer/DefaultExecutorTest.*` to verify that the modified tests ran properly. Thanks, Jeff Coffler
Re: Review Request 63253: Fixed DefaultExecutorTest for proper URI handling on Windows.
> On Oct. 25, 2017, 4:38 p.m., Gaston Kleiman wrote: > > src/tests/default_executor_tests.cpp > > Lines 1650 (patched) > > <https://reviews.apache.org/r/63253/diff/1/?file=1867546#file1867546line1653> > > > > This should be: > > > > `" if ((Get-Content testFile) -NotMatch 'pizza')"` > > > > To make sure that the file is copied to the sandbox, and that the > > task's working dir is the sandbox. > > Andrew Schwartzmeyer wrote: > This leads into a larger problem on Windows: `shell` tasks are run under > `cmd.exe`, which as a legacy app does not support long paths (actually, it > doesn't even recognize them, it thinks they're UNC paths are bails), so it > falls back to a temp directory. Until we've swapped the default shell on > Windows from `cmd.exe` to `powershell.exe`, this is a problem. The task _is_ > started in the sandbox, but `cmd.exe` then changes directory :( > > All that said, we went the route of using `testFilePath`, the full path > to the file ensure its contents are correct (that the file was copied right). > But you're right, I think it makes more sense to ensure the task was started > in the right place, and then test it relatively. > > Jeff, this means to go back to using `set_shell(false)` etc. to > `powershell.exe` directly, instead of under `cmd.exe`. Yup, I knew that when I discussed this with Gaston. All set with this. - Jeff --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63253/#review189204 --- On Oct. 24, 2017, 6:04 p.m., Jeff Coffler wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/63253/ > --- > > (Updated Oct. 24, 2017, 6: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 > --- > > Fixed changes from Gaston (git commit 772c8f5 on 10/02/2017) > for proper operation on Windows. Changed to use new URI methods > introduced as part of MESOS-6705, and used powershell on Windows > to replace "test" command on Linux. > > > Diffs > - > > src/tests/default_executor_tests.cpp > 5078bd4d70698f5cbd14c971fcecfd58f8467a04 > > > Diff: https://reviews.apache.org/r/63253/diff/1/ > > > Testing > --- > > Ran mesos-tests on both Windows and Linux with no errors. > > Specifically ran mesos-tests with > `--gtest_filter=MesosContainerizer/DefaultExecutorTest.*` to verify that the > modified tests ran properly. > > > Thanks, > > Jeff Coffler > >
Re: Review Request 63253: Fixed DefaultExecutorTest for proper URI handling on Windows.
> On Oct. 25, 2017, 3:58 p.m., Gaston Kleiman wrote: > > src/tests/default_executor_tests.cpp > > Lines 1764 (patched) > > <https://reviews.apache.org/r/63253/diff/1/?file=1867546#file1867546line1771> > > > > The indentation here looks strange. This was removed since it's specific to Linux (alpine image). > On Oct. 25, 2017, 3:58 p.m., Gaston Kleiman wrote: > > src/tests/default_executor_tests.cpp > > Lines 1766-1770 (patched) > > <https://reviews.apache.org/r/63253/diff/1/?file=1867546#file1867546line1773> > > > > I don't think this will work inside an `alpine` Docker container, will > > it? Removed since this isn't appropriate for alpine container. I missed that the first time, thanks for pointing it out! - Jeff --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63253/#review189201 --- On Oct. 24, 2017, 6:04 p.m., Jeff Coffler wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/63253/ > --- > > (Updated Oct. 24, 2017, 6: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 > --- > > Fixed changes from Gaston (git commit 772c8f5 on 10/02/2017) > for proper operation on Windows. Changed to use new URI methods > introduced as part of MESOS-6705, and used powershell on Windows > to replace "test" command on Linux. > > > Diffs > - > > src/tests/default_executor_tests.cpp > 5078bd4d70698f5cbd14c971fcecfd58f8467a04 > > > Diff: https://reviews.apache.org/r/63253/diff/1/ > > > Testing > --- > > Ran mesos-tests on both Windows and Linux with no errors. > > Specifically ran mesos-tests with > `--gtest_filter=MesosContainerizer/DefaultExecutorTest.*` to verify that the > modified tests ran properly. > > > Thanks, > > Jeff Coffler > >
Re: Review Request 63253: Fixed DefaultExecutorTest for proper URI handling on Windows.
> On Oct. 25, 2017, 5:25 p.m., Andrew Schwartzmeyer wrote: > > src/tests/default_executor_tests.cpp > > Line 1766 (original), 1772 (patched) > > <https://reviews.apache.org/r/63253/diff/1/?file=1867546#file1867546line1779> > > > > Is it just me or did this indentation change? I don't see that. It's indented two spaces, and it used to be indented two spaces. Do you see something different? - Jeff --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63253/#review189210 ------- On Oct. 24, 2017, 6:04 p.m., Jeff Coffler wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/63253/ > --- > > (Updated Oct. 24, 2017, 6: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 > --- > > Fixed changes from Gaston (git commit 772c8f5 on 10/02/2017) > for proper operation on Windows. Changed to use new URI methods > introduced as part of MESOS-6705, and used powershell on Windows > to replace "test" command on Linux. > > > Diffs > - > > src/tests/default_executor_tests.cpp > 5078bd4d70698f5cbd14c971fcecfd58f8467a04 > > > Diff: https://reviews.apache.org/r/63253/diff/1/ > > > Testing > --- > > Ran mesos-tests on both Windows and Linux with no errors. > > Specifically ran mesos-tests with > `--gtest_filter=MesosContainerizer/DefaultExecutorTest.*` to verify that the > modified tests ran properly. > > > Thanks, > > Jeff Coffler > >
Review Request 63253: Fixed DefaultExecutorTest for proper URI handling on Windows.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63253/ --- 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 --- Fixed changes from Gaston (git commit 772c8f5 on 10/02/2017) for proper operation on Windows. Changed to use new URI methods introduced as part of MESOS-6705, and used powershell on Windows to replace "test" command on Linux. Diffs - src/tests/default_executor_tests.cpp 5078bd4d70698f5cbd14c971fcecfd58f8467a04 Diff: https://reviews.apache.org/r/63253/diff/1/ Testing --- Ran mesos-tests on both Windows and Linux with no errors. Specifically ran mesos-tests with `--gtest_filter=MesosContainerizer/DefaultExecutorTest.*` to verify that the modified tests ran properly. Thanks, Jeff Coffler
Re: Review Request 60620: Modifed os::write to write binary files on Windows.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60620/ --- (Updated Oct. 19, 2017, 9:47 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 --- By default, ::write operations on Windows will include a \r\n (CR/LF) at the end of each line, which is not compatible with Linux. Mesos expects only \n (LF) characters between lines. This commit will modify Windows behavior to only include \n at the end of each line written to a text file. Diffs - 3rdparty/stout/include/stout/os/write.hpp 9ff749f209e6dd6ca3695907108a029c9a2b4f05 Diff: https://reviews.apache.org/r/60620/diff/7/ Testing --- Built successfully on both Linux (with autotools and cmake) and Windows (with cmake). Ran stout-tests and mesos-tests successfully on both Windows and Linux. Thanks, Jeff Coffler
Re: Review Request 60620: Modifed os::write to write binary files on Windows.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60620/ --- (Updated Oct. 19, 2017, 9:34 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 --- By default, ::write operations on Windows will include a \r\n (CR/LF) at the end of each line, which is not compatible with Linux. Mesos expects only \n (LF) characters between lines. This commit will modify Windows behavior to only include \n at the end of each line written to a text file. Diffs (updated) - 3rdparty/stout/include/stout/os/write.hpp 9ff749f209e6dd6ca3695907108a029c9a2b4f05 Diff: https://reviews.apache.org/r/60620/diff/7/ Changes: https://reviews.apache.org/r/60620/diff/6-7/ Testing --- Built successfully on both Linux (with autotools and cmake) and Windows (with cmake). Ran stout-tests and mesos-tests successfully on both Windows and Linux. Thanks, Jeff Coffler
Re: Review Request 60628: Enabled fetcher_tests.cpp unit test module on Windows platform.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60628/ --- (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 --- 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 (updated) - 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/6/ Changes: https://reviews.apache.org/r/60628/diff/5-6/ Testing --- See upstream. Thanks, Jeff Coffler
Re: Review Request 60624: Enabled HDFS compilation and associated tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60624/ --- (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 --- Note that HDFS tests are disabled for Windows due to dependence on 'sh' shell. Be aware: HDFS hasn't been tested, although it should work. We will formally add support for HDFS at a later date, see MESOS-5460. Diffs (updated) - src/CMakeLists.txt 219252f6f82b2d62d024b2ab876fa0ba2f5c8e6c src/tests/CMakeLists.txt 386e0473c93d0a993248c7818067071d0c761c76 src/tests/hdfs_tests.cpp e7154c75e663d9a98bec48be42a59b65c96f9515 Diff: https://reviews.apache.org/r/60624/diff/6/ Changes: https://reviews.apache.org/r/60624/diff/5-6/ Testing --- See upstream Thanks, Jeff Coffler
Re: Review Request 60622: Added new stout functions for path and URI conversions.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60622/ --- (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 --- Added new stout function: path::from_uri, which converts to a filepath from a URI. Also added uri::from_path, which generates a URI from a file path. Diffs (updated) - 3rdparty/stout/Makefile.am 4386017acd6ca465be3f735460c11d50b440ccc5 3rdparty/stout/include/Makefile.am bdd3e9908ebfc682458a3babc34cbee36ad3f751 3rdparty/stout/include/stout/path.hpp 6ee3a44cd6a878fe383aa68df40b82857b93d0b4 3rdparty/stout/include/stout/uri.hpp PRE-CREATION 3rdparty/stout/tests/CMakeLists.txt 6e5773f1e03671de7ac007caf49edd0f1cd7aedd 3rdparty/stout/tests/path_tests.cpp f8c14d5aefe0b49adb778da784143a328c96183d 3rdparty/stout/tests/uri_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/60622/diff/6/ Changes: https://reviews.apache.org/r/60622/diff/5-6/ Testing --- See upstream Thanks, Jeff Coffler
Re: Review Request 60626: Eliminated os::shell calls from HDFS for Windows compatibility.
--- 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 60623: Converted "file://" URI handling to use new uri function.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60623/ --- (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 --- Converted "file://" URI handling to use new uri function. Diffs (updated) - src/tests/credentials_tests.cpp 7eb5abd21a1be35d4739c4928cb922f028cfc9a7 src/tests/fetcher_tests.cpp df9d2d9586a6457004506c4e2a972ccfc912c7c5 src/tests/master_contender_detector_tests.cpp f499136c0b981072af5bc8accf2238b7ba4bf430 src/tests/script.cpp 8d40e01da005cb05e7804f0b3975e3e0edb8f3bd Diff: https://reviews.apache.org/r/60623/diff/6/ Changes: https://reviews.apache.org/r/60623/diff/5-6/ Testing --- See upstream Thanks, Jeff Coffler
Re: Review Request 60621: Added new stout capability: os::copyfile(source, dest).
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60621/ --- (Updated Oct. 19, 2017, 9: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 --- The os::copyfile() method provides an API to copy a file from one location to another. Note that copying of directories are not supported, and that the destination directory must exist before a file can be copied into a directory. Diffs (updated) - 3rdparty/stout/Makefile.am 4386017acd6ca465be3f735460c11d50b440ccc5 3rdparty/stout/include/Makefile.am bdd3e9908ebfc682458a3babc34cbee36ad3f751 3rdparty/stout/include/stout/os/copyfile.hpp PRE-CREATION 3rdparty/stout/include/stout/os/posix/copyfile.hpp PRE-CREATION 3rdparty/stout/include/stout/os/windows/copyfile.hpp PRE-CREATION 3rdparty/stout/tests/CMakeLists.txt 6e5773f1e03671de7ac007caf49edd0f1cd7aedd 3rdparty/stout/tests/os/copyfile_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/60621/diff/6/ Changes: https://reviews.apache.org/r/60621/diff/5-6/ Testing --- See upstream Note that Joe made some changes to this, I ended up taking his changes as is. Thanks, Jeff Coffler
Re: Review Request 60624: Enabled HDFS compilation and associated tests.
> On Oct. 17, 2017, 11:24 p.m., Andrew Schwartzmeyer wrote: > > src/tests/hdfs_tests.cpp > > Lines 55-57 (original), 55-60 (patched) > > <https://reviews.apache.org/r/60624/diff/4/?file=1858872#file1858872line55> > > > > Nit: this comment isn't great... we do know how to handle execution > > permissions on Windows. As in, the correct thing to do is not to mark it > > executable, as the concept simply doesn't map. I disagree. Yes, it doesn't map, but there certainly isn't concensus in how to handle this. We have both positive and negative tests dealing with the execution bit. I personally think the "right" solution is to have a run-time test that tells us if the platform supports that (I tend to prefer runtime tests to #ifdef's because they are often compiled out anyway, but will catch syntax errors that #ifdef's would miss). Then a test can "do the right thing" based on the platform and the test can pass everywhere (both on platforms with an execution bit and on platforms without an execution bit). But, being an open source project, I need to write something up for this, distribute it to Mesos DEVs, and get buy-in. Until we have that, we don't know exactly how we're going to handle this. This is on my list of things to do, and I will do it. But other DEVs may have other ideas on how to handle this properly. - Jeff --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60624/#review188418 --- On Oct. 17, 2017, 1:18 a.m., Jeff Coffler wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60624/ > --- > > (Updated Oct. 17, 2017, 1:18 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 HDFS tests are disabled for Windows due to dependence on > 'sh' shell. > > Be aware: HDFS hasn't been tested, although it should work. We will > formally add support for HDFS at a later date, see MESOS-5460. > > > Diffs > - > > src/CMakeLists.txt 1a0dff3604c288d2af8ecc23f1ec73f5a350bc31 > src/tests/CMakeLists.txt 386e0473c93d0a993248c7818067071d0c761c76 > src/tests/hdfs_tests.cpp e7154c75e663d9a98bec48be42a59b65c96f9515 > > > Diff: https://reviews.apache.org/r/60624/diff/4/ > > > Testing > --- > > See upstream > > > Thanks, > > Jeff Coffler > >
Re: Review Request 60624: Enabled HDFS compilation and associated tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60624/ --- (Updated Oct. 19, 2017, 6:18 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 HDFS tests are disabled for Windows due to dependence on 'sh' shell. Be aware: HDFS hasn't been tested, although it should work. We will formally add support for HDFS at a later date, see MESOS-5460. Diffs (updated) - src/CMakeLists.txt 1a0dff3604c288d2af8ecc23f1ec73f5a350bc31 src/tests/CMakeLists.txt 386e0473c93d0a993248c7818067071d0c761c76 src/tests/hdfs_tests.cpp e7154c75e663d9a98bec48be42a59b65c96f9515 Diff: https://reviews.apache.org/r/60624/diff/5/ Changes: https://reviews.apache.org/r/60624/diff/4-5/ Testing --- See upstream Thanks, Jeff Coffler
Re: Review Request 60628: Enabled fetcher_tests.cpp unit test module on Windows platform.
> 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 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, 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 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
Re: Review Request 60628: Enabled fetcher_tests.cpp unit test module on Windows platform.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60628/ --- (Updated Oct. 19, 2017, 6:18 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 --- 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 (updated) - 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/5/ Changes: https://reviews.apache.org/r/60628/diff/4-5/ Testing --- See upstream. Thanks, Jeff Coffler
Re: Review Request 60626: Eliminated os::shell calls from HDFS for Windows compatibility.
--- 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 60623: Converted "file://" URI handling to use new uri function.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60623/ --- (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 --- Converted "file://" URI handling to use new uri function. Diffs (updated) - src/tests/credentials_tests.cpp 7eb5abd21a1be35d4739c4928cb922f028cfc9a7 src/tests/fetcher_tests.cpp df9d2d9586a6457004506c4e2a972ccfc912c7c5 src/tests/master_contender_detector_tests.cpp f499136c0b981072af5bc8accf2238b7ba4bf430 src/tests/script.cpp 8d40e01da005cb05e7804f0b3975e3e0edb8f3bd Diff: https://reviews.apache.org/r/60623/diff/5/ Changes: https://reviews.apache.org/r/60623/diff/4-5/ Testing --- See upstream Thanks, Jeff Coffler
Re: Review Request 60626: Eliminated os::shell calls from HDFS for Windows compatibility.
> 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) > > <https://reviews.apache.org/r/60626/diff/4/?file=1858873#file1858873line125> > > > > 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 60622: Added new stout functions for path and URI conversions.
> On Oct. 17, 2017, 11:21 p.m., Andrew Schwartzmeyer wrote: > > 3rdparty/stout/include/stout/path.hpp > > Lines 45-52 (patched) > > <https://reviews.apache.org/r/60622/diff/4/?file=1858861#file1858861line45> > > > > The `startsWith` is unecessary. This can be replaced with one line: > > > > ``` > > std::string path = strings::remove(path, "file://", strings::PREFIX);` > > ``` > > > > Since `strings::remove` is idempotent. Changed. > On Oct. 17, 2017, 11:21 p.m., Andrew Schwartzmeyer wrote: > > 3rdparty/stout/include/stout/path.hpp > > Lines 47 (patched) > > <https://reviews.apache.org/r/60622/diff/4/?file=1858861#file1858861line47> > > > > nit: comments must end with `.` Ok. > On Oct. 17, 2017, 11:21 p.m., Andrew Schwartzmeyer wrote: > > 3rdparty/stout/include/stout/path.hpp > > Lines 48 (patched) > > <https://reviews.apache.org/r/60622/diff/4/?file=1858861#file1858861line48> > > > > nit: whitespace Ok. > On Oct. 17, 2017, 11:21 p.m., Andrew Schwartzmeyer wrote: > > 3rdparty/stout/include/stout/path.hpp > > Lines 49-50 (patched) > > <https://reviews.apache.org/r/60622/diff/4/?file=1858861#file1858861line49> > > > > nit: open brace should be on line with `if` Refactored out due to prior change. > On Oct. 17, 2017, 11:21 p.m., Andrew Schwartzmeyer wrote: > > 3rdparty/stout/include/stout/uri.hpp > > Lines 24 (patched) > > <https://reviews.apache.org/r/60622/diff/4/?file=1858862#file1858862line24> > > > > nit: comment must end with a `.`, and this should perhaps mention why > > we replace `` with `/` on Windows. Ok. - Jeff ------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60622/#review188414 --- On Oct. 17, 2017, 1:17 a.m., Jeff Coffler wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60622/ > --- > > (Updated Oct. 17, 2017, 1: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 > --- > > Added new stout function: path::from_uri, which converts to a filepath > from a URI. > > Also added uri::from_path, which generates a URI from a file path. > > > Diffs > - > > 3rdparty/stout/Makefile.am 4386017acd6ca465be3f735460c11d50b440ccc5 > 3rdparty/stout/include/Makefile.am bdd3e9908ebfc682458a3babc34cbee36ad3f751 > 3rdparty/stout/include/stout/path.hpp > 6ee3a44cd6a878fe383aa68df40b82857b93d0b4 > 3rdparty/stout/include/stout/uri.hpp PRE-CREATION > 3rdparty/stout/tests/CMakeLists.txt > 6e5773f1e03671de7ac007caf49edd0f1cd7aedd > 3rdparty/stout/tests/path_tests.cpp > f8c14d5aefe0b49adb778da784143a328c96183d > 3rdparty/stout/tests/uri_tests.cpp PRE-CREATION > > > Diff: https://reviews.apache.org/r/60622/diff/4/ > > > Testing > --- > > See upstream > > > Thanks, > > Jeff Coffler > >
Re: Review Request 60622: Added new stout functions for path and URI conversions.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60622/ --- (Updated Oct. 19, 2017, 6:16 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 --- Added new stout function: path::from_uri, which converts to a filepath from a URI. Also added uri::from_path, which generates a URI from a file path. Diffs (updated) - 3rdparty/stout/Makefile.am 4386017acd6ca465be3f735460c11d50b440ccc5 3rdparty/stout/include/Makefile.am bdd3e9908ebfc682458a3babc34cbee36ad3f751 3rdparty/stout/include/stout/path.hpp 6ee3a44cd6a878fe383aa68df40b82857b93d0b4 3rdparty/stout/include/stout/uri.hpp PRE-CREATION 3rdparty/stout/tests/CMakeLists.txt 6e5773f1e03671de7ac007caf49edd0f1cd7aedd 3rdparty/stout/tests/path_tests.cpp f8c14d5aefe0b49adb778da784143a328c96183d 3rdparty/stout/tests/uri_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/60622/diff/5/ Changes: https://reviews.apache.org/r/60622/diff/4-5/ Testing --- See upstream Thanks, Jeff Coffler
Re: Review Request 60621: Added new stout capability: os::copyfile(source, dest).
> On Oct. 17, 2017, 11:16 p.m., Andrew Schwartzmeyer wrote: > > 3rdparty/stout/include/stout/os/posix/copyfile.hpp > > Lines 38 (patched) > > <https://reviews.apache.org/r/60621/diff/4/?file=1858855#file1858855line38> > > > > nit: whitespace Removed blank line. > On Oct. 17, 2017, 11:16 p.m., Andrew Schwartzmeyer wrote: > > 3rdparty/stout/include/stout/os/posix/copyfile.hpp > > Lines 45-50 (patched) > > <https://reviews.apache.org/r/60621/diff/4/?file=1858855#file1858855line45> > > > > nit: indentation Emacs default, sorry. Fixed. > On Oct. 17, 2017, 11:16 p.m., Andrew Schwartzmeyer wrote: > > 3rdparty/stout/include/stout/os/windows/copyfile.hpp > > Lines 37 (patched) > > <https://reviews.apache.org/r/60621/diff/4/?file=1858856#file1858856line37> > > > > nit: whitespace Fixed. > On Oct. 17, 2017, 11:16 p.m., Andrew Schwartzmeyer wrote: > > 3rdparty/stout/include/stout/os/windows/copyfile.hpp > > Lines 51-55 (patched) > > <https://reviews.apache.org/r/60621/diff/4/?file=1858856#file1858856line51> > > > > nit: indentation Fixed. > On Oct. 17, 2017, 11:16 p.m., Andrew Schwartzmeyer wrote: > > 3rdparty/stout/tests/os/copyfile_tests.cpp > > Lines 79 (patched) > > <https://reviews.apache.org/r/60621/diff/4/?file=1858858#file1858858line79> > > > > nit: indentation Fixed. - Jeff --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60621/#review188413 --- On Oct. 17, 2017, 1:16 a.m., Jeff Coffler wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60621/ > --- > > (Updated Oct. 17, 2017, 1:16 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 > --- > > The os::copyfile() method provides an API to copy a file from one > location to another. Note that copying of directories are not > supported, and that the destination directory must exist before a > file can be copied into a directory. > > > Diffs > - > > 3rdparty/stout/Makefile.am 4386017acd6ca465be3f735460c11d50b440ccc5 > 3rdparty/stout/include/Makefile.am bdd3e9908ebfc682458a3babc34cbee36ad3f751 > 3rdparty/stout/include/stout/os/copyfile.hpp PRE-CREATION > 3rdparty/stout/include/stout/os/posix/copyfile.hpp PRE-CREATION > 3rdparty/stout/include/stout/os/windows/copyfile.hpp PRE-CREATION > 3rdparty/stout/tests/CMakeLists.txt > 6e5773f1e03671de7ac007caf49edd0f1cd7aedd > 3rdparty/stout/tests/os/copyfile_tests.cpp PRE-CREATION > > > Diff: https://reviews.apache.org/r/60621/diff/4/ > > > Testing > --- > > See upstream > > Note that Joe made some changes to this, I ended up taking his changes as is. > > > Thanks, > > Jeff Coffler > >
Re: Review Request 60621: Added new stout capability: os::copyfile(source, dest).
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60621/ --- (Updated Oct. 19, 2017, 6:16 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 --- The os::copyfile() method provides an API to copy a file from one location to another. Note that copying of directories are not supported, and that the destination directory must exist before a file can be copied into a directory. Diffs (updated) - 3rdparty/stout/Makefile.am 4386017acd6ca465be3f735460c11d50b440ccc5 3rdparty/stout/include/Makefile.am bdd3e9908ebfc682458a3babc34cbee36ad3f751 3rdparty/stout/include/stout/os/copyfile.hpp PRE-CREATION 3rdparty/stout/include/stout/os/posix/copyfile.hpp PRE-CREATION 3rdparty/stout/include/stout/os/windows/copyfile.hpp PRE-CREATION 3rdparty/stout/tests/CMakeLists.txt 6e5773f1e03671de7ac007caf49edd0f1cd7aedd 3rdparty/stout/tests/os/copyfile_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/60621/diff/5/ Changes: https://reviews.apache.org/r/60621/diff/4-5/ Testing --- See upstream Note that Joe made some changes to this, I ended up taking his changes as is. Thanks, Jeff Coffler
Re: Review Request 60620: Modifed os::write to write binary files on Windows.
> On Oct. 17, 2017, 11:12 p.m., Andrew Schwartzmeyer wrote: > > 3rdparty/stout/include/stout/os/write.hpp > > Lines 107 (patched) > > <https://reviews.apache.org/r/60620/diff/5/?file=1858851#file1858851line107> > > > > nit: extra whitespace introduced Removed blank line. - Jeff --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60620/#review188412 --- On Oct. 17, 2017, 1:16 a.m., Jeff Coffler wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60620/ > --- > > (Updated Oct. 17, 2017, 1:16 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 > --- > > By default, ::write operations on Windows will include a \r\n (CR/LF) > at the end of each line, which is not compatible with Linux. Mesos > expects only \n (LF) characters between lines. > > This commit will modify Windows behavior to only include \n at the > end of each line written to a text file. > > > Diffs > - > > 3rdparty/stout/include/stout/os/write.hpp > 9ff749f209e6dd6ca3695907108a029c9a2b4f05 > > > Diff: https://reviews.apache.org/r/60620/diff/5/ > > > Testing > --- > > Built successfully on both Linux (with autotools and cmake) and Windows (with > cmake). > > Ran stout-tests and mesos-tests successfully on both Windows and Linux. > > > Thanks, > > Jeff Coffler > >
Re: Review Request 60620: Modifed os::write to write binary files on Windows.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60620/ --- (Updated Oct. 19, 2017, 6:15 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 --- By default, ::write operations on Windows will include a \r\n (CR/LF) at the end of each line, which is not compatible with Linux. Mesos expects only \n (LF) characters between lines. This commit will modify Windows behavior to only include \n at the end of each line written to a text file. Diffs (updated) - 3rdparty/stout/include/stout/os/write.hpp 9ff749f209e6dd6ca3695907108a029c9a2b4f05 Diff: https://reviews.apache.org/r/60620/diff/6/ Changes: https://reviews.apache.org/r/60620/diff/5-6/ Testing --- Built successfully on both Linux (with autotools and cmake) and Windows (with cmake). Ran stout-tests and mesos-tests successfully on both Windows and Linux. Thanks, Jeff Coffler
Re: Review Request 60628: Enabled fetcher_tests.cpp unit test module on Windows platform.
> On Oct. 13, 2017, 7:28 p.m., Andrew Schwartzmeyer wrote: > > src/slave/containerizer/fetcher.cpp > > Lines 202-213 (original), 202-210 (patched) > > <https://reviews.apache.org/r/60628/diff/3/?file=1852896#file1852896line202> > > > > This logic should be refactored into `Try path::from_uri`. Refactored into std::string path::from_uri (since that function can't return an error per our discussion). Today, URIs and paths are used interchangably within MESOS, not not prepending a "file://" URI prefix will not trigger an error. - Jeff --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60628/#review188003 ------- 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 > >
Re: Review Request 60624: Enabled HDFS compilation and associated tests.
> On Oct. 13, 2017, 7:20 p.m., Andrew Schwartzmeyer wrote: > > src/launcher/fetcher.cpp > > Lines 267-272 (original), 267-275 (patched) > > <https://reviews.apache.org/r/60624/diff/3/?file=1852890#file1852890line267> > > > > The changes to `fetcher.cpp` shouldn't be in this review, please move > > them to #60628. This was originally done as part of the work to bring HDFS onboard, so it was checked in with that. But I'll merge with the other commit for the bulk of the fetcher work. - Jeff --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60624/#review188002 ------- On Oct. 17, 2017, 1:18 a.m., Jeff Coffler wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60624/ > --- > > (Updated Oct. 17, 2017, 1:18 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 HDFS tests are disabled for Windows due to dependence on > 'sh' shell. > > Be aware: HDFS hasn't been tested, although it should work. We will > formally add support for HDFS at a later date, see MESOS-5460. > > > Diffs > - > > src/CMakeLists.txt 1a0dff3604c288d2af8ecc23f1ec73f5a350bc31 > src/tests/CMakeLists.txt 386e0473c93d0a993248c7818067071d0c761c76 > src/tests/hdfs_tests.cpp e7154c75e663d9a98bec48be42a59b65c96f9515 > > > Diff: https://reviews.apache.org/r/60624/diff/4/ > > > Testing > --- > > See upstream > > > Thanks, > > Jeff Coffler > >
Re: Review Request 60626: Eliminated os::shell calls from HDFS for Windows compatibility.
--- 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 60624: Enabled HDFS compilation and associated tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60624/ --- (Updated Oct. 17, 2017, 1:18 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 HDFS tests are disabled for Windows due to dependence on 'sh' shell. Be aware: HDFS hasn't been tested, although it should work. We will formally add support for HDFS at a later date, see MESOS-5460. Diffs (updated) - src/CMakeLists.txt 1a0dff3604c288d2af8ecc23f1ec73f5a350bc31 src/tests/CMakeLists.txt 386e0473c93d0a993248c7818067071d0c761c76 src/tests/hdfs_tests.cpp e7154c75e663d9a98bec48be42a59b65c96f9515 Diff: https://reviews.apache.org/r/60624/diff/4/ Changes: https://reviews.apache.org/r/60624/diff/3-4/ Testing --- See upstream Thanks, Jeff Coffler
Re: Review Request 60628: Enabled fetcher_tests.cpp unit test module on Windows platform.
--- 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 (updated) - 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/ Changes: https://reviews.apache.org/r/60628/diff/3-4/ Testing --- See upstream. Thanks, Jeff Coffler
