> On Feb. 8, 2017, 1:08 a.m., Vinod Kone wrote: > > src/launcher/executor.cpp, lines 714-753 > > <https://reviews.apache.org/r/56210/diff/1/?file=1622060#file1622060line714> > > > > I think these helpers are bit confusing. For example, it's not clear to > > me when I should use `update()` vs `bootstrappedUpdate()` when I write new > > code in the future that generates a status update (say TASK_STARTING). The > > comment on top of `update()` seems too specific to health checks for the > > generic function names that you picked here. > > > > Is `update` supposed to be used when `TaskState` changes whereas the > > `bootstrappedUpdate` should be used when it doesn't? I hope not because > > even if TaskState changes you would want to preserve health and check > > status. Right now it kinda works because TASK_RUNNING is the only state > > that can have different health / check statuses, whereas TASK_KILLING or > > TASK_FINISHED or TASK_FINISHED don't need that info. > > > > Alternatively, can we just merge these 2 into one helper that takes a > > bunch of optional fields that can overwrite fields in `lastTaskStatus`? > > Would that be more intuitive?
I think there is value in having two helpers: one for creating an update from scratch and one for creating an update from a previous one. We can merge them, but I'd prefer having them separate because they clear express the intent. Now, it's hard to say when to use one and when the other. It's up to the framework's writer whether they want to preserve certain data or not. I doubt we should tell people to use the bootstrapped version by default, since it implies sending more data over the network. I think currently we should leave this decision to an executor writer and come up with a guideline in the nearest future. I have a related RR here: https://reviews.apache.org/r/56017/ and will plan to start a thread on the devlist about it. What we should do though, is to explain what these two helpers do so that executor authors can use them wisely. Does this sound like a good plan to you? - Alexander ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56210/#review164584 ----------------------------------------------------------- On Feb. 8, 2017, 4:56 p.m., Alexander Rukletsov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56210/ > ----------------------------------------------------------- > > (Updated Feb. 8, 2017, 4:56 p.m.) > > > Review request for mesos, Gastón Kleiman and Vinod Kone. > > > Bugs: MESOS-6906 > https://issues.apache.org/jira/browse/MESOS-6906 > > > Repository: mesos > > > Description > ------- > > When a new task status update is generated in the executor, we have > to make sure specific data is duplicated from the previous update > to, e.g., avoid shadowing of those data during reconciliation. For > instance, consider a check status being sent; in this status update > we must include the latest known health information. > > > Diffs > ----- > > src/launcher/executor.cpp d9417ce1d5b108f5292a66010eab80f11552a969 > > Diff: https://reviews.apache.org/r/56210/diff/ > > > Testing > ------- > > See https://reviews.apache.org/r/56218/ > > > Thanks, > > Alexander Rukletsov > >
