You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jie Yu <yu...@gmail.com> on 2017/08/24 23:58:32 UTC
Review Request 61901: Added common validation for Volume.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61901/
-----------------------------------------------------------
Review request for mesos, Gilbert Song and Joseph Wu.
Bugs: MESOS-7306
https://issues.apache.org/jira/browse/MESOS-7306
Repository: mesos
Description
-------
Added common validation for Volume.
Diffs
-----
src/Makefile.am 5157b2baac4ff6c4493faaeb7a311381e8e76fe7
src/common/validation.hpp 8c92436eb6c8aa87ffd6d6cf3f42079d6c90e525
src/common/validation.cpp 33b501bfa6613dab70a663f552a424ffa8a15b0a
src/tests/CMakeLists.txt 6c05ea93c3288fb9608c191234f95e270390d89e
src/tests/common_validation_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/61901/diff/1/
Testing
-------
make check
Thanks,
Jie Yu
Re: Review Request 61901: Added common validation for Volume.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61901/#review184023
-----------------------------------------------------------
Ship it!
Ship It!
- Joseph Wu
On Aug. 28, 2017, 8:28 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61901/
> -----------------------------------------------------------
>
> (Updated Aug. 28, 2017, 8:28 p.m.)
>
>
> Review request for mesos, Gilbert Song and Joseph Wu.
>
>
> Bugs: MESOS-7306
> https://issues.apache.org/jira/browse/MESOS-7306
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added common validation for Volume.
>
>
> Diffs
> -----
>
> src/Makefile.am 5157b2baac4ff6c4493faaeb7a311381e8e76fe7
> src/common/validation.hpp 8c92436eb6c8aa87ffd6d6cf3f42079d6c90e525
> src/common/validation.cpp 33b501bfa6613dab70a663f552a424ffa8a15b0a
> src/tests/CMakeLists.txt 6c05ea93c3288fb9608c191234f95e270390d89e
> src/tests/common_validation_tests.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/61901/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 61901: Added common validation for Volume.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61901/
-----------------------------------------------------------
(Updated Aug. 29, 2017, 3:28 a.m.)
Review request for mesos, Gilbert Song and Joseph Wu.
Changes
-------
addressed review comments.
Bugs: MESOS-7306
https://issues.apache.org/jira/browse/MESOS-7306
Repository: mesos
Description
-------
Added common validation for Volume.
Diffs (updated)
-----
src/Makefile.am 5157b2baac4ff6c4493faaeb7a311381e8e76fe7
src/common/validation.hpp 8c92436eb6c8aa87ffd6d6cf3f42079d6c90e525
src/common/validation.cpp 33b501bfa6613dab70a663f552a424ffa8a15b0a
src/tests/CMakeLists.txt 6c05ea93c3288fb9608c191234f95e270390d89e
src/tests/common_validation_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/61901/diff/2/
Changes: https://reviews.apache.org/r/61901/diff/1-2/
Testing
-------
make check
Thanks,
Jie Yu
Re: Review Request 61901: Added common validation for Volume.
Posted by Jie Yu <yu...@gmail.com>.
> On Aug. 28, 2017, 7:11 p.m., Joseph Wu wrote:
> > src/common/validation.cpp
> > Lines 206 (patched)
> > <https://reviews.apache.org/r/61901/diff/1/?file=1803367#file1803367line206>
> >
> > Seems like you could start with adding this:
> > ```
> > if (!path::absolute(volume.host_path()) &&
> > !path::absolute(volume.container_path())) {
> > ...
> > }
> > ```
> >
> > (Some form of which exists in the filesystem/linux isolator and Docker containerizer)
I'll do that later.
> On Aug. 28, 2017, 7:11 p.m., Joseph Wu wrote:
> > src/common/validation.cpp
> > Lines 212-217 (patched)
> > <https://reviews.apache.org/r/61901/diff/1/?file=1803367#file1803367line212>
> >
> > Might be clearer to count the number of occurrences rather than bit-mask them.
> >
> > Then error if the number of occurrences is not equal to one.
> >
> > ---
> >
> > Also, I'm surprised that we don't have this validation already.
> >
> > It seems like, in the existing code, if you:
> > 1) Specify some value for all three fields.
> > 2) Enable the appropriate isolators (filesystem/linux, docker/volume, volume/sandbox_path).
> > 3) All three isolators will do their thing, possibly running over each other in the process.
Used occurrences! thanks for the tip.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61901/#review183968
-----------------------------------------------------------
On Aug. 24, 2017, 11:58 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61901/
> -----------------------------------------------------------
>
> (Updated Aug. 24, 2017, 11:58 p.m.)
>
>
> Review request for mesos, Gilbert Song and Joseph Wu.
>
>
> Bugs: MESOS-7306
> https://issues.apache.org/jira/browse/MESOS-7306
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added common validation for Volume.
>
>
> Diffs
> -----
>
> src/Makefile.am 5157b2baac4ff6c4493faaeb7a311381e8e76fe7
> src/common/validation.hpp 8c92436eb6c8aa87ffd6d6cf3f42079d6c90e525
> src/common/validation.cpp 33b501bfa6613dab70a663f552a424ffa8a15b0a
> src/tests/CMakeLists.txt 6c05ea93c3288fb9608c191234f95e270390d89e
> src/tests/common_validation_tests.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/61901/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 61901: Added common validation for Volume.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61901/#review183968
-----------------------------------------------------------
src/common/validation.cpp
Lines 206 (patched)
<https://reviews.apache.org/r/61901/#comment260012>
Seems like you could start with adding this:
```
if (!path::absolute(volume.host_path()) &&
!path::absolute(volume.container_path())) {
...
}
```
(Some form of which exists in the filesystem/linux isolator and Docker containerizer)
src/common/validation.cpp
Lines 212-217 (patched)
<https://reviews.apache.org/r/61901/#comment260010>
Might be clearer to count the number of occurrences rather than bit-mask them.
Then error if the number of occurrences is not equal to one.
---
Also, I'm surprised that we don't have this validation already.
It seems like, in the existing code, if you:
1) Specify some value for all three fields.
2) Enable the appropriate isolators (filesystem/linux, docker/volume, volume/sandbox_path).
3) All three isolators will do their thing, possibly running over each other in the process.
src/common/validation.cpp
Lines 249-250 (patched)
<https://reviews.apache.org/r/61901/#comment260007>
The default case should be an error on its own. You could leave out the default case so that this code won't compile when a new Source type is defined.
- Joseph Wu
On Aug. 24, 2017, 4:58 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61901/
> -----------------------------------------------------------
>
> (Updated Aug. 24, 2017, 4:58 p.m.)
>
>
> Review request for mesos, Gilbert Song and Joseph Wu.
>
>
> Bugs: MESOS-7306
> https://issues.apache.org/jira/browse/MESOS-7306
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added common validation for Volume.
>
>
> Diffs
> -----
>
> src/Makefile.am 5157b2baac4ff6c4493faaeb7a311381e8e76fe7
> src/common/validation.hpp 8c92436eb6c8aa87ffd6d6cf3f42079d6c90e525
> src/common/validation.cpp 33b501bfa6613dab70a663f552a424ffa8a15b0a
> src/tests/CMakeLists.txt 6c05ea93c3288fb9608c191234f95e270390d89e
> src/tests/common_validation_tests.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/61901/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>