> 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 > >
