> On Feb. 8, 2017, 1:08 a.m., Vinod Kone wrote: > > src/launcher/executor.cpp, line 720 > > <https://reviews.apache.org/r/56210/diff/1/?file=1622060#file1622060line720> > > > > looks like this review doesn't use `reason` argument, so I wouldn't add > > it in this review. lets add it in the review that needs it. > > Alexander Rukletsov wrote: > A'd rather keep it here, because I would like API to look consistent. > What are general things people should probably set if they generate a status > update? State, reason, message. They are not specific to any particular > update or feature (e.g., check update).
"I would like API to look consistent" : Consistent with what? It's not clear to me why someone creating a status update only cares about state/reason/message. That seems very specific to this use case. If I have the executor sending a TASK_STARTING update for example, I cannot use either of these helpers as they exist in this review. > 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? > > Alexander Rukletsov wrote: > 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? I agree that having 2 different helpers might be useful, but the current signatures (names and args) of these helpers are not intuitive to me. If you want to tackle the problem of right helper function in r/56017, I would rather you avoid creating this helper in this review and just inline the code with a TODO. I really want to avoid introducing non-intuitive helper functions. - Vinod ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56210/#review164584 ----------------------------------------------------------- On Feb. 9, 2017, 12:56 p.m., Alexander Rukletsov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56210/ > ----------------------------------------------------------- > > (Updated Feb. 9, 2017, 12: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 > >
