> On Dec. 24, 2014, 1:20 a.m., Adam B wrote: > > src/slave/slave.cpp, lines 2669-2676 > > <https://reviews.apache.org/r/28063/diff/5/?file=799499#file799499line2669> > > > > There should only be one CommandInfo to modify, so you can use a CHECK > > or make this an if/else > > Alexander Rukletsov wrote: > There is only one place where we enforce this constraint. I do not want > to introduce this check in another piece of code, since the behaviour may > change. Current grace period update code will handle both situations > correctly.
Fair enough. Dropping. But maybe we should have a protobuf utility function that takes a TaskInfo and returns a reference to the valid CommandInfo? > On Dec. 24, 2014, 1:20 a.m., Adam B wrote: > > src/slave/slave.cpp, lines 2661-2664 > > <https://reviews.apache.org/r/28063/diff/5/?file=799499#file799499line2661> > > > > Why not take `flags` as a parameter too? Then setGracePeriod(task, > > flags) or setGracePeriod(task, grace_period) could be a static method that > > doesn't even need to live on `Slave`. > > Alexander Rukletsov wrote: > This sounds reasonable. I suggest to introduce `enforceGracePeriod(task, > grace_period)` in `common/protobuf_utils.{hpp|cpp}`. Does it make sense? Do we really expect this method to be used anywhere outside of this one place in slave.cpp? If so, let's move it to protobuf_utils. If not, let's keep it hidden in slave.cpp until it becomes more widely used. > On Dec. 24, 2014, 1:20 a.m., Adam B wrote: > > src/slave/slave.hpp, line 276 > > <https://reviews.apache.org/r/28063/diff/5/?file=799498#file799498line276> > > > > private? > > Alexander Rukletsov wrote: > I put it into the "pseudo-protected" section, together to other similar > methods. In private section we have mostly fields and utility functions. Does > it make sense or would you still go for private? Dropping. This function no longer needs to be a method on Slave, as discussed below. > On Dec. 24, 2014, 1:20 a.m., Adam B wrote: > > include/mesos/mesos.proto, line 257 > > <https://reviews.apache.org/r/28063/diff/5/?file=799497#file799497line257> > > > > grace_period_seconds? To make the unit explicit > > Is there a default, or does it default to None? > > Alexander Rukletsov wrote: > There is a default in slave flags, but I don't think it makes sense to > repeat it here. I'll change the name to `grace_period_seconds`. I was concerned about the upgrade path for this optional field. Any older frameworks/masters sending a TaskInfo to a newer slave will not include this field, so it will default to None. Looks like slave's set/enforceGracePeriod function will handle an empty grace period by overwriting it with the flag value. Should work fine. Rename SGTM. > On Dec. 24, 2014, 1:20 a.m., Adam B wrote: > > src/slave/slave.cpp, lines 1159-1160 > > <https://reviews.apache.org/r/28063/diff/5/?file=799499#file799499line1159> > > > > I feel like 'setGracePeriod' is inaccurate if it doesn't always set it > > (when already set). Maybe 'updateGracePeriod' or 'enforceGracePeriod' or > > 'mergeGracePeriod'? You can probably come up with something better. > > Alexander Rukletsov wrote: > Currently, the function always sets the grace period. However, this may > change in the future. I tend to agree and go for `enforceGracePeriod()`. What > do you think? Call it overrideGracePeriod until you fix that TODO and allow already-set values to survive. At that point, you can rename it to something more accurate (I still think there's gotta be a better name out there). - Adam ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28063/#review66019 ----------------------------------------------------------- On Dec. 23, 2014, 7:25 a.m., Alexander Rukletsov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/28063/ > ----------------------------------------------------------- > > (Updated Dec. 23, 2014, 7:25 a.m.) > > > Review request for mesos, Ben Mahler, Niklas Nielsen, and Till Toenshoff. > > > Bugs: MESOS-1571 > https://issues.apache.org/jira/browse/MESOS-1571 > > > Repository: mesos-git > > > Description > ------- > > CommandExecutor grace_period field is designed to be used by slave to > propagate the value of the grace period flag further to containerizers and > executors. > > > Diffs > ----- > > include/mesos/mesos.proto 0085aba > src/slave/slave.hpp 70bd8c1 > src/slave/slave.cpp ed63ded > > Diff: https://reviews.apache.org/r/28063/diff/ > > > Testing > ------- > > make check (Mac OS 10.9.4, Ubuntu 14.04) > > > Thanks, > > Alexander Rukletsov > >
