You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Chi Zhang <ch...@gmail.com> on 2015/03/06 02:26:59 UTC
Review Request 31784: type_utils: Relaxened the equality check of
CommandInfo to allow 'unset' environment == 'empty' environment.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31784/
-----------------------------------------------------------
Review request for mesos, Alexander Rukletsov, Joerg Schad, Till Toenshoff, Vinod Kone, and Zameer Manji.
Bugs: mesos-2309
https://issues.apache.org/jira/browse/mesos-2309
Repository: mesos
Description
-------
type_utils: Relaxened the equality check of CommandInfo to allow 'unset' environment == 'empty' environment.
Diffs
-----
src/Makefile.am d299f07d865080676ca8a550cf6005c6ab32839f
src/common/type_utils.cpp a1704c67d04d19f65d94dbe56a61bb28561e5bf3
src/tests/type_utils_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/31784/diff/
Testing
-------
Thanks,
Chi Zhang
Re: Review Request 31784: type_utils: Relaxened the equality check of
CommandInfo to allow 'unset' environment == 'empty' environment.
Posted by Joerg Schad <jo...@mesosphere.io>.
> On March 9, 2015, 9:51 p.m., Joerg Schad wrote:
> > src/common/type_utils.cpp, line 59
> > <https://reviews.apache.org/r/31784/diff/2/?file=887439#file887439line59>
> >
> > From my understanding this will cause errors as environment does not have a default value and hence we should check before whether it is present (see https://code.google.com/p/protobuf-c/wiki/Default_Values).
retracted.
- Joerg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31784/#review75793
-----------------------------------------------------------
On March 6, 2015, 6:04 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31784/
> -----------------------------------------------------------
>
> (Updated March 6, 2015, 6:04 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Joerg Schad, Till Toenshoff, Vinod Kone, and Zameer Manji.
>
>
> Bugs: mesos-2309
> https://issues.apache.org/jira/browse/mesos-2309
>
>
> Repository: mesos
>
>
> Description
> -------
>
> type_utils: Relaxened the equality check of CommandInfo to allow 'unset' environment == 'empty' environment.
>
>
> Diffs
> -----
>
> src/Makefile.am d299f07d865080676ca8a550cf6005c6ab32839f
> src/common/type_utils.cpp a1704c67d04d19f65d94dbe56a61bb28561e5bf3
> src/tests/type_utils_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/31784/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 31784: type_utils: Relaxened the equality check of
CommandInfo to allow 'unset' environment == 'empty' environment.
Posted by Joerg Schad <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31784/#review75793
-----------------------------------------------------------
src/common/type_utils.cpp
<https://reviews.apache.org/r/31784/#comment123080>
From my understanding this will cause errors as environment does not have a default value and hence we should check before whether it is present (see https://code.google.com/p/protobuf-c/wiki/Default_Values).
- Joerg Schad
On March 6, 2015, 6:04 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31784/
> -----------------------------------------------------------
>
> (Updated March 6, 2015, 6:04 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Joerg Schad, Till Toenshoff, Vinod Kone, and Zameer Manji.
>
>
> Bugs: mesos-2309
> https://issues.apache.org/jira/browse/mesos-2309
>
>
> Repository: mesos
>
>
> Description
> -------
>
> type_utils: Relaxened the equality check of CommandInfo to allow 'unset' environment == 'empty' environment.
>
>
> Diffs
> -----
>
> src/Makefile.am d299f07d865080676ca8a550cf6005c6ab32839f
> src/common/type_utils.cpp a1704c67d04d19f65d94dbe56a61bb28561e5bf3
> src/tests/type_utils_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/31784/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 31784: type_utils: Relaxened the equality check of
CommandInfo to allow 'unset' environment == 'empty' environment.
Posted by Zameer Manji <zm...@apache.org>.
> On March 6, 2015, 10:18 a.m., Vinod Kone wrote:
> > src/common/type_utils.cpp, line 59
> > <https://reviews.apache.org/r/31784/diff/2/?file=887439#file887439line59>
> >
> > I don't think doing this change piece-wise for one variable (e.g., 'environment') at a time is intuitive to readers of this code.
> >
> > We need to decide if we want to change comparing 'optional' messages everywhere. FWIW, with proto3 this will no longer be an issue, because there are neither optionals nor defaults.
>
> Alexander Rukletsov wrote:
> Agreed with Vinod, this desrves a more general approach. The difference between this case and `shell` case here: https://reviews.apache.org/r/31011/ is that the latter has a default. What you try to enforce here is that absence is equivalent to empty instance. This seems reasonable for this case (and many other) but it's not universally true. A quick question: why do you explicitly set empty env for some tasks and leave it unset for other?
In addition, a general approach would make understanding the API much easier. Framework writers will not need to consult the equality code in the mesos code to determine if wire level changes will cause rejections.
- Zameer
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31784/#review75502
-----------------------------------------------------------
On March 6, 2015, 10:04 a.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31784/
> -----------------------------------------------------------
>
> (Updated March 6, 2015, 10:04 a.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Joerg Schad, Till Toenshoff, Vinod Kone, and Zameer Manji.
>
>
> Bugs: mesos-2309
> https://issues.apache.org/jira/browse/mesos-2309
>
>
> Repository: mesos
>
>
> Description
> -------
>
> type_utils: Relaxened the equality check of CommandInfo to allow 'unset' environment == 'empty' environment.
>
>
> Diffs
> -----
>
> src/Makefile.am d299f07d865080676ca8a550cf6005c6ab32839f
> src/common/type_utils.cpp a1704c67d04d19f65d94dbe56a61bb28561e5bf3
> src/tests/type_utils_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/31784/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 31784: type_utils: Relaxened the equality check of
CommandInfo to allow 'unset' environment == 'empty' environment.
Posted by Alexander Rukletsov <al...@mesosphere.io>.
> On March 6, 2015, 6:18 p.m., Vinod Kone wrote:
> > src/common/type_utils.cpp, line 59
> > <https://reviews.apache.org/r/31784/diff/2/?file=887439#file887439line59>
> >
> > I don't think doing this change piece-wise for one variable (e.g., 'environment') at a time is intuitive to readers of this code.
> >
> > We need to decide if we want to change comparing 'optional' messages everywhere. FWIW, with proto3 this will no longer be an issue, because there are neither optionals nor defaults.
Agreed with Vinod, this desrves a more general approach. The difference between this case and `shell` case here: https://reviews.apache.org/r/31011/ is that the latter has a default. What you try to enforce here is that absence is equivalent to empty instance. This seems reasonable for this case (and many other) but it's not universally true. A quick question: why do you explicitly set empty env for some tasks and leave it unset for other?
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31784/#review75502
-----------------------------------------------------------
On March 6, 2015, 6:04 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31784/
> -----------------------------------------------------------
>
> (Updated March 6, 2015, 6:04 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Joerg Schad, Till Toenshoff, Vinod Kone, and Zameer Manji.
>
>
> Bugs: mesos-2309
> https://issues.apache.org/jira/browse/mesos-2309
>
>
> Repository: mesos
>
>
> Description
> -------
>
> type_utils: Relaxened the equality check of CommandInfo to allow 'unset' environment == 'empty' environment.
>
>
> Diffs
> -----
>
> src/Makefile.am d299f07d865080676ca8a550cf6005c6ab32839f
> src/common/type_utils.cpp a1704c67d04d19f65d94dbe56a61bb28561e5bf3
> src/tests/type_utils_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/31784/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 31784: type_utils: Relaxened the equality check of
CommandInfo to allow 'unset' environment == 'empty' environment.
Posted by Joerg Schad <jo...@mesosphere.io>.
> On March 6, 2015, 6:18 p.m., Vinod Kone wrote:
> > src/common/type_utils.cpp, line 59
> > <https://reviews.apache.org/r/31784/diff/2/?file=887439#file887439line59>
> >
> > I don't think doing this change piece-wise for one variable (e.g., 'environment') at a time is intuitive to readers of this code.
> >
> > We need to decide if we want to change comparing 'optional' messages everywhere. FWIW, with proto3 this will no longer be an issue, because there are neither optionals nor defaults.
>
> Alexander Rukletsov wrote:
> Agreed with Vinod, this desrves a more general approach. The difference between this case and `shell` case here: https://reviews.apache.org/r/31011/ is that the latter has a default. What you try to enforce here is that absence is equivalent to empty instance. This seems reasonable for this case (and many other) but it's not universally true. A quick question: why do you explicitly set empty env for some tasks and leave it unset for other?
>
> Zameer Manji wrote:
> In addition, a general approach would make understanding the API much easier. Framework writers will not need to consult the equality code in the mesos code to determine if wire level changes will cause rejections.
@Vinod: +1 for a standard approach to comparing messages, we should distinguish between optional with default (shell) value and optional withput default value (environment). proto3 sounds cool!
- Joerg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31784/#review75502
-----------------------------------------------------------
On March 6, 2015, 6:04 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31784/
> -----------------------------------------------------------
>
> (Updated March 6, 2015, 6:04 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Joerg Schad, Till Toenshoff, Vinod Kone, and Zameer Manji.
>
>
> Bugs: mesos-2309
> https://issues.apache.org/jira/browse/mesos-2309
>
>
> Repository: mesos
>
>
> Description
> -------
>
> type_utils: Relaxened the equality check of CommandInfo to allow 'unset' environment == 'empty' environment.
>
>
> Diffs
> -----
>
> src/Makefile.am d299f07d865080676ca8a550cf6005c6ab32839f
> src/common/type_utils.cpp a1704c67d04d19f65d94dbe56a61bb28561e5bf3
> src/tests/type_utils_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/31784/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 31784: type_utils: Relaxened the equality check of
CommandInfo to allow 'unset' environment == 'empty' environment.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31784/#review75502
-----------------------------------------------------------
src/common/type_utils.cpp
<https://reviews.apache.org/r/31784/#comment122622>
I don't think doing this change piece-wise for one variable (e.g., 'environment') at a time is intuitive to readers of this code.
We need to decide if we want to change comparing 'optional' messages everywhere. FWIW, with proto3 this will no longer be an issue, because there are neither optionals nor defaults.
- Vinod Kone
On March 6, 2015, 6:04 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31784/
> -----------------------------------------------------------
>
> (Updated March 6, 2015, 6:04 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Joerg Schad, Till Toenshoff, Vinod Kone, and Zameer Manji.
>
>
> Bugs: mesos-2309
> https://issues.apache.org/jira/browse/mesos-2309
>
>
> Repository: mesos
>
>
> Description
> -------
>
> type_utils: Relaxened the equality check of CommandInfo to allow 'unset' environment == 'empty' environment.
>
>
> Diffs
> -----
>
> src/Makefile.am d299f07d865080676ca8a550cf6005c6ab32839f
> src/common/type_utils.cpp a1704c67d04d19f65d94dbe56a61bb28561e5bf3
> src/tests/type_utils_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/31784/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 31784: type_utils: Relaxened the equality check of
CommandInfo to allow 'unset' environment == 'empty' environment.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31784/#review75533
-----------------------------------------------------------
Patch looks great!
Reviews applied: [31784]
All tests passed.
- Mesos ReviewBot
On March 6, 2015, 6:04 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31784/
> -----------------------------------------------------------
>
> (Updated March 6, 2015, 6:04 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Joerg Schad, Till Toenshoff, Vinod Kone, and Zameer Manji.
>
>
> Bugs: mesos-2309
> https://issues.apache.org/jira/browse/mesos-2309
>
>
> Repository: mesos
>
>
> Description
> -------
>
> type_utils: Relaxened the equality check of CommandInfo to allow 'unset' environment == 'empty' environment.
>
>
> Diffs
> -----
>
> src/Makefile.am d299f07d865080676ca8a550cf6005c6ab32839f
> src/common/type_utils.cpp a1704c67d04d19f65d94dbe56a61bb28561e5bf3
> src/tests/type_utils_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/31784/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 31784: type_utils: Relaxened the equality check of
CommandInfo to allow 'unset' environment == 'empty' environment.
Posted by Chi Zhang <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31784/
-----------------------------------------------------------
(Updated March 6, 2015, 6:04 p.m.)
Review request for mesos, Alexander Rukletsov, Joerg Schad, Till Toenshoff, Vinod Kone, and Zameer Manji.
Bugs: mesos-2309
https://issues.apache.org/jira/browse/mesos-2309
Repository: mesos
Description
-------
type_utils: Relaxened the equality check of CommandInfo to allow 'unset' environment == 'empty' environment.
Diffs (updated)
-----
src/Makefile.am d299f07d865080676ca8a550cf6005c6ab32839f
src/common/type_utils.cpp a1704c67d04d19f65d94dbe56a61bb28561e5bf3
src/tests/type_utils_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/31784/diff/
Testing
-------
Thanks,
Chi Zhang
Re: Review Request 31784: type_utils: Relaxened the equality check of
CommandInfo to allow 'unset' environment == 'empty' environment.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31784/#review75458
-----------------------------------------------------------
Patch looks great!
Reviews applied: [31784]
All tests passed.
- Mesos ReviewBot
On March 6, 2015, 1:26 a.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31784/
> -----------------------------------------------------------
>
> (Updated March 6, 2015, 1:26 a.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Joerg Schad, Till Toenshoff, Vinod Kone, and Zameer Manji.
>
>
> Bugs: mesos-2309
> https://issues.apache.org/jira/browse/mesos-2309
>
>
> Repository: mesos
>
>
> Description
> -------
>
> type_utils: Relaxened the equality check of CommandInfo to allow 'unset' environment == 'empty' environment.
>
>
> Diffs
> -----
>
> src/Makefile.am d299f07d865080676ca8a550cf6005c6ab32839f
> src/common/type_utils.cpp a1704c67d04d19f65d94dbe56a61bb28561e5bf3
> src/tests/type_utils_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/31784/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Chi Zhang
>
>