You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Andrew Schwartzmeyer <an...@schwartzmeyer.com> on 2018/08/10 17:26:09 UTC
Review Request 68297: Windows: Fixed a bug when `executorId` contains
a `:`.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68297/
-----------------------------------------------------------
Review request for mesos, Benjamin Mahler, Gastón Kleiman, and Joseph Wu.
Bugs: MESOS-9109
https://issues.apache.org/jira/browse/MESOS-9109
Repository: mesos
Description
-------
Per MESOS-9109, some frameworks, such as Chronos, use a `:` in their
`executorId`, but this is a reserved character on Windows, and when
its use as part of the path on disk results in an OS error.
Unfortunately, we need to remain backward-compatible with existing
frameworks, so now we escape `:` as `_COLON_` when writing to disk,
and unescape it when parsing.
Note that we explicitly do not escape to `%3A` (the ASCII equivalent),
because otherwise it is impossible to query for the file over HTTP, as
libprocess helpfully (but not in this case) pre-emptively decodes it
to `:` and so queries for the incorrect path.
Diffs
-----
src/slave/paths.cpp ed0b1276908f4990ce7a24c96aea20e8c79d3126
src/tests/paths_tests.cpp 4808402f789ebc783b308c03b8e412ba22bf9f5d
Diff: https://reviews.apache.org/r/68297/diff/1/
Testing
-------
```
[----------] Global test environment tear-down
[==========] 1008 tests from 98 test cases ran. (446260 ms total)
[ PASSED ] 1008 tests.
```
Note that this is without changing the used-everywhere default executor ID of `default` to `default:id`, which I also tested, but required changes of asserts (and caused long path issues only solvable by changing the machine's registry, so I'd rather not include it).
Thanks,
Andrew Schwartzmeyer
Re: Review Request 68297: Windows: Fixed a bug when `executorId`
contains a `:`.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68297/
-----------------------------------------------------------
(Updated Aug. 17, 2018, 6:18 p.m.)
Review request for mesos, Benjamin Mahler, Gastón Kleiman, and Joseph Wu.
Bugs: MESOS-9109
https://issues.apache.org/jira/browse/MESOS-9109
Repository: mesos
Description (updated)
-------
Per MESOS-9109, some frameworks, such as Chronos, use a `:` in their
`executorId`, but this is a reserved character on Windows, and when
its used as part of the path on disk results in an OS error.
Unfortunately, we need to remain backward-compatible with existing
frameworks, so now we escape `:` as `%3A` when writing to disk, and
unescape it when parsing.
Diffs (updated)
-----
src/slave/paths.cpp ed0b1276908f4990ce7a24c96aea20e8c79d3126
src/tests/paths_tests.cpp 4808402f789ebc783b308c03b8e412ba22bf9f5d
Diff: https://reviews.apache.org/r/68297/diff/3/
Changes: https://reviews.apache.org/r/68297/diff/2-3/
Testing
-------
```
[----------] Global test environment tear-down
[==========] 1008 tests from 98 test cases ran. (446260 ms total)
[ PASSED ] 1008 tests.
```
Note that this is without changing the used-everywhere default executor ID of `default` to `default:id`, which I also tested, but required changes of asserts (and caused long path issues only solvable by changing the machine's registry, so I'd rather not include it).
Thanks,
Andrew Schwartzmeyer
Re: Review Request 68297: Windows: Fixed a bug when `executorId`
contains a `:`.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68297/
-----------------------------------------------------------
(Updated Aug. 15, 2018, 8:12 p.m.)
Review request for mesos, Benjamin Mahler, Gastón Kleiman, and Joseph Wu.
Changes
-------
Switched to `%3A`. TODO: Maybe use `process::http::encode` here instead.
Bugs: MESOS-9109
https://issues.apache.org/jira/browse/MESOS-9109
Repository: mesos
Description
-------
Per MESOS-9109, some frameworks, such as Chronos, use a `:` in their
`executorId`, but this is a reserved character on Windows, and when
its use as part of the path on disk results in an OS error.
Unfortunately, we need to remain backward-compatible with existing
frameworks, so now we escape `:` as `_COLON_` when writing to disk,
and unescape it when parsing.
Note that we explicitly do not escape to `%3A` (the ASCII equivalent),
because otherwise it is impossible to query for the file over HTTP, as
libprocess helpfully (but not in this case) pre-emptively decodes it
to `:` and so queries for the incorrect path.
Diffs (updated)
-----
src/slave/paths.cpp ed0b1276908f4990ce7a24c96aea20e8c79d3126
src/tests/paths_tests.cpp 4808402f789ebc783b308c03b8e412ba22bf9f5d
Diff: https://reviews.apache.org/r/68297/diff/2/
Changes: https://reviews.apache.org/r/68297/diff/1-2/
Testing
-------
```
[----------] Global test environment tear-down
[==========] 1008 tests from 98 test cases ran. (446260 ms total)
[ PASSED ] 1008 tests.
```
Note that this is without changing the used-everywhere default executor ID of `default` to `default:id`, which I also tested, but required changes of asserts (and caused long path issues only solvable by changing the machine's registry, so I'd rather not include it).
Thanks,
Andrew Schwartzmeyer
Re: Review Request 68297: Windows: Fixed a bug when `executorId`
contains a `:`.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68297/#review207084
-----------------------------------------------------------
PASS: Mesos patch 68297 was successfully built and tested.
Reviews applied: `['68297']`
All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2128/mesos-review-68297
- Mesos Reviewbot Windows
On Aug. 10, 2018, 5:26 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68297/
> -----------------------------------------------------------
>
> (Updated Aug. 10, 2018, 5:26 p.m.)
>
>
> Review request for mesos, Benjamin Mahler, Gastón Kleiman, and Joseph Wu.
>
>
> Bugs: MESOS-9109
> https://issues.apache.org/jira/browse/MESOS-9109
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Per MESOS-9109, some frameworks, such as Chronos, use a `:` in their
> `executorId`, but this is a reserved character on Windows, and when
> its use as part of the path on disk results in an OS error.
> Unfortunately, we need to remain backward-compatible with existing
> frameworks, so now we escape `:` as `_COLON_` when writing to disk,
> and unescape it when parsing.
>
> Note that we explicitly do not escape to `%3A` (the ASCII equivalent),
> because otherwise it is impossible to query for the file over HTTP, as
> libprocess helpfully (but not in this case) pre-emptively decodes it
> to `:` and so queries for the incorrect path.
>
>
> Diffs
> -----
>
> src/slave/paths.cpp ed0b1276908f4990ce7a24c96aea20e8c79d3126
> src/tests/paths_tests.cpp 4808402f789ebc783b308c03b8e412ba22bf9f5d
>
>
> Diff: https://reviews.apache.org/r/68297/diff/1/
>
>
> Testing
> -------
>
> ```
> [----------] Global test environment tear-down
> [==========] 1008 tests from 98 test cases ran. (446260 ms total)
> [ PASSED ] 1008 tests.
> ```
>
> Note that this is without changing the used-everywhere default executor ID of `default` to `default:id`, which I also tested, but required changes of asserts (and caused long path issues only solvable by changing the machine's registry, so I'd rather not include it).
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>