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/10/27 10:26:05 UTC
Review Request 63355: Added validation for disk related new
operations.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63355/
-----------------------------------------------------------
Review request for mesos, Benjamin Bannier, Gaston Kleiman, Greg Mann, and Jan Schlicht.
Repository: mesos
Description
-------
(This is based on https://reviews.apache.org/r/61946)
Diffs
-----
src/master/validation.hpp f4925752f20ae8ca4de1d9b4a3d5ffc394db9585
src/master/validation.cpp 42f5742386b59a983f7caaf3400c82b7ef4f8bbb
src/tests/master_validation_tests.cpp 7da1be5233444aded64263d4a763fe2967459042
Diff: https://reviews.apache.org/r/63355/diff/1/
Testing
-------
make check
Thanks,
Jie Yu
Re: Review Request 63355: Added validation for disk related new
operations.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63355/#review189421
-----------------------------------------------------------
Patch looks great!
Reviews applied: [63001, 62903, 63094, 63104, 63254, 63255, 63256, 63257, 63355]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On Oct. 27, 2017, 10:26 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63355/
> -----------------------------------------------------------
>
> (Updated Oct. 27, 2017, 10:26 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, Gaston Kleiman, Greg Mann, and Jan Schlicht.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> (This is based on https://reviews.apache.org/r/61946)
>
>
> Diffs
> -----
>
> src/master/validation.hpp f4925752f20ae8ca4de1d9b4a3d5ffc394db9585
> src/master/validation.cpp 42f5742386b59a983f7caaf3400c82b7ef4f8bbb
> src/tests/master_validation_tests.cpp 7da1be5233444aded64263d4a763fe2967459042
>
>
> Diff: https://reviews.apache.org/r/63355/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 63355: Added validation for disk related new
operations.
Posted by Benjamin Bannier <bb...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63355/#review189591
-----------------------------------------------------------
Ship it!
Ship It!
- Benjamin Bannier
On Oct. 30, 2017, 4:07 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63355/
> -----------------------------------------------------------
>
> (Updated Oct. 30, 2017, 4:07 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Gaston Kleiman, Greg Mann, and Jan Schlicht.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> (This is based on https://reviews.apache.org/r/61946)
>
>
> Diffs
> -----
>
> src/master/validation.hpp f4925752f20ae8ca4de1d9b4a3d5ffc394db9585
> src/master/validation.cpp 42f5742386b59a983f7caaf3400c82b7ef4f8bbb
> src/tests/master_validation_tests.cpp 7da1be5233444aded64263d4a763fe2967459042
>
>
> Diff: https://reviews.apache.org/r/63355/diff/3/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 63355: Added validation for disk related new
operations.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63355/
-----------------------------------------------------------
(Updated Oct. 30, 2017, 3:07 p.m.)
Review request for mesos, Benjamin Bannier, Gaston Kleiman, Greg Mann, and Jan Schlicht.
Changes
-------
Addressed comments.
Repository: mesos
Description
-------
(This is based on https://reviews.apache.org/r/61946)
Diffs (updated)
-----
src/master/validation.hpp f4925752f20ae8ca4de1d9b4a3d5ffc394db9585
src/master/validation.cpp 42f5742386b59a983f7caaf3400c82b7ef4f8bbb
src/tests/master_validation_tests.cpp 7da1be5233444aded64263d4a763fe2967459042
Diff: https://reviews.apache.org/r/63355/diff/3/
Changes: https://reviews.apache.org/r/63355/diff/2-3/
Testing
-------
make check
Thanks,
Jie Yu
Re: Review Request 63355: Added validation for disk related new
operations.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63355/#review189522
-----------------------------------------------------------
Patch looks great!
Reviews applied: [63001, 62903, 63094, 63104, 63254, 63255, 63256, 63257, 63355]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On Oct. 29, 2017, 3:47 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63355/
> -----------------------------------------------------------
>
> (Updated Oct. 29, 2017, 3:47 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Gaston Kleiman, Greg Mann, and Jan Schlicht.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> (This is based on https://reviews.apache.org/r/61946)
>
>
> Diffs
> -----
>
> src/master/validation.hpp f4925752f20ae8ca4de1d9b4a3d5ffc394db9585
> src/master/validation.cpp 42f5742386b59a983f7caaf3400c82b7ef4f8bbb
> src/tests/master_validation_tests.cpp 7da1be5233444aded64263d4a763fe2967459042
>
>
> Diff: https://reviews.apache.org/r/63355/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 63355: Added validation for disk related new
operations.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63355/#review189537
-----------------------------------------------------------
PASS: Mesos patch 63355 was successfully built and tested.
Reviews applied: `['63104', '63254', '63255', '63256', '63257', '63355']`
All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/63355
- Mesos Reviewbot Windows
On Oct. 29, 2017, 11:47 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63355/
> -----------------------------------------------------------
>
> (Updated Oct. 29, 2017, 11:47 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Gaston Kleiman, Greg Mann, and Jan Schlicht.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> (This is based on https://reviews.apache.org/r/61946)
>
>
> Diffs
> -----
>
> src/master/validation.hpp f4925752f20ae8ca4de1d9b4a3d5ffc394db9585
> src/master/validation.cpp 42f5742386b59a983f7caaf3400c82b7ef4f8bbb
> src/tests/master_validation_tests.cpp 7da1be5233444aded64263d4a763fe2967459042
>
>
> Diff: https://reviews.apache.org/r/63355/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 63355: Added validation for disk related new
operations.
Posted by Jie Yu <yu...@gmail.com>.
> On Oct. 30, 2017, 10:21 a.m., Benjamin Bannier wrote:
> > src/master/validation.cpp
> > Lines 2279 (patched)
> > <https://reviews.apache.org/r/63355/diff/2/?file=1872373#file1872373line2279>
> >
> > Since the implementations of the validations for `CreateVolume/DestroyVolume` and `CreateBlock/DestroyBlock` are identical I wonder whether it makes sense to instead introduce two reusable basic validation functions instead of four. We could still expose four functions to users.
I'd punt that for now. Will revisit if the validation here becomes bigger.
> On Oct. 30, 2017, 10:21 a.m., Benjamin Bannier wrote:
> > src/master/validation.cpp
> > Lines 2296-2297 (patched)
> > <https://reviews.apache.org/r/63355/diff/2/?file=1872373#file1872373line2296>
> >
> > I feel this would be better handled with a `switch` explicitly naming all understood types.
I think this is easy to read comparing to a switch.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63355/#review189556
-----------------------------------------------------------
On Oct. 29, 2017, 3:47 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63355/
> -----------------------------------------------------------
>
> (Updated Oct. 29, 2017, 3:47 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Gaston Kleiman, Greg Mann, and Jan Schlicht.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> (This is based on https://reviews.apache.org/r/61946)
>
>
> Diffs
> -----
>
> src/master/validation.hpp f4925752f20ae8ca4de1d9b4a3d5ffc394db9585
> src/master/validation.cpp 42f5742386b59a983f7caaf3400c82b7ef4f8bbb
> src/tests/master_validation_tests.cpp 7da1be5233444aded64263d4a763fe2967459042
>
>
> Diff: https://reviews.apache.org/r/63355/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 63355: Added validation for disk related new
operations.
Posted by Benjamin Bannier <bb...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63355/#review189556
-----------------------------------------------------------
src/master/validation.cpp
Lines 2279 (patched)
<https://reviews.apache.org/r/63355/#comment266686>
Since the implementations of the validations for `CreateVolume/DestroyVolume` and `CreateBlock/DestroyBlock` are identical I wonder whether it makes sense to instead introduce two reusable basic validation functions instead of four. We could still expose four functions to users.
src/master/validation.cpp
Lines 2285 (patched)
<https://reviews.apache.org/r/63355/#comment266684>
nit: `error->message`.
src/master/validation.cpp
Lines 2296-2297 (patched)
<https://reviews.apache.org/r/63355/#comment266682>
I feel this would be better handled with a `switch` explicitly naming all understood types.
src/master/validation.cpp
Lines 2311 (patched)
<https://reviews.apache.org/r/63355/#comment266685>
nit: `error->message`.
src/master/validation.cpp
Lines 2318-2319 (patched)
<https://reviews.apache.org/r/63355/#comment266683>
I feel this would be better handled with a `switch` explicitly naming all understood types.
- Benjamin Bannier
On Oct. 29, 2017, 4:47 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63355/
> -----------------------------------------------------------
>
> (Updated Oct. 29, 2017, 4:47 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Gaston Kleiman, Greg Mann, and Jan Schlicht.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> (This is based on https://reviews.apache.org/r/61946)
>
>
> Diffs
> -----
>
> src/master/validation.hpp f4925752f20ae8ca4de1d9b4a3d5ffc394db9585
> src/master/validation.cpp 42f5742386b59a983f7caaf3400c82b7ef4f8bbb
> src/tests/master_validation_tests.cpp 7da1be5233444aded64263d4a763fe2967459042
>
>
> Diff: https://reviews.apache.org/r/63355/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 63355: Added validation for disk related new
operations.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63355/
-----------------------------------------------------------
(Updated Oct. 29, 2017, 3:47 p.m.)
Review request for mesos, Benjamin Bannier, Gaston Kleiman, Greg Mann, and Jan Schlicht.
Changes
-------
Rebased.
Repository: mesos
Description
-------
(This is based on https://reviews.apache.org/r/61946)
Diffs (updated)
-----
src/master/validation.hpp f4925752f20ae8ca4de1d9b4a3d5ffc394db9585
src/master/validation.cpp 42f5742386b59a983f7caaf3400c82b7ef4f8bbb
src/tests/master_validation_tests.cpp 7da1be5233444aded64263d4a763fe2967459042
Diff: https://reviews.apache.org/r/63355/diff/2/
Changes: https://reviews.apache.org/r/63355/diff/1-2/
Testing
-------
make check
Thanks,
Jie Yu