----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15542/#review32234 -----------------------------------------------------------
Thanks for taking on this API change Jason, they can be a bit of a pain to flush out fully but we're pretty close here! A few things of note: 1. What is the required upgrade order here? From my observation: -> Slaves must be upgraded first (in order to correctly receive the new 'argv' style tasks). -> Masters and Schedulers have no upgrade order required. We'll need to keep track of this somewhere (0.17.0 JIRA or docs/Upgrades.md) so that users know how to upgrade safely (this will land in 0.17.0). 2. I'd like us to make the initial step to moving 'value' to optional, by modifying the scheduler driver to set 'value' to an empty string when 'command' is set. This would eliminate the need for frameworks to set 'value' to an empty string in a subsequent release. include/mesos/mesos.proto <https://reviews.apache.org/r/15542/#comment60973> Can you s/uri/URI/ in this comment? include/mesos/mesos.proto <https://reviews.apache.org/r/15542/#comment60974> We handle other file extensions as well, we should update this comment to reflect all of the extensions we handle. include/mesos/mesos.proto <https://reviews.apache.org/r/15542/#comment60967> s/bytes/string on these two The resulting 'executor.command.command.command' is a bit unfortunate, do you foresee other fields in Command? If not, let's just directly embed it in CommandInfo: message CommandInfo { // Setting value is equivalent to: // command = 'sh' // arguments = ['-c', value] // // Deprecated, please use 'command' and 'arguments' instead. required string value = 3; optional string command = 4; repeated string arguments = 5; } It seems that this makes the API a bit cleaner to use, how does this sound? include/mesos/mesos.proto <https://reviews.apache.org/r/15542/#comment60961> We should try to move this to an optional field to make the semantics more clear to frameworks, but we'll need to go through a few release cycles to do this. We would have compatibility issues if we were to move this to optional immediately, since messages originating from an upgraded framework will not be parseable my the Master / Slave. 1. As a first step, let's modify sched.cpp to set 'value' to an empty string when 'command' is set. This will ensure that when upgrading to a subsequent release, all messages coming from the scheduler driver will have the field set. 2. Subsequently, and let's leave TODOs for this, we'll be able to move this safely to an optional field. Removing it entirely would be a bit trickier, since it's a breaking API change, I would recommend linking a ticket into MESOS-810 to capture this dependency. Sound good? - Ben Mahler On Jan. 17, 2014, 5:54 a.m., Jason Dusek wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/15542/ > ----------------------------------------------------------- > > (Updated Jan. 17, 2014, 5:54 a.m.) > > > Review request for mesos, Benjamin Hindman, Ben Mahler, and Niklas Nielsen. > > > Repository: mesos-git > > > Description > ------- > > Offer an execvp like interface for running tasks. > > Review: https://reviews.apache.org/r/15542 > > > Diffs > ----- > > include/mesos/mesos.proto 655f86757487ddbe551fdcf53eb793e773ecdd34 > src/examples/python/test_framework.py > deca48e779ae099424fa73bb9a8ac5c419c5faf1 > src/launcher/executor.cpp b73ab479500a7347a38ba53acecfab9229f1080d > src/launcher/launcher.cpp d5ab66704429a95eeb8eda5188e33d8e691221af > src/launcher/main.cpp de64609905ee63096c0173fe7e64a1eafea5d6bf > src/slave/process_isolator.cpp 0bc698f04f7c8eaad166dc9d646e13310129dd01 > > Diff: https://reviews.apache.org/r/15542/diff/ > > > Testing > ------- > > Ran Python test executor and `make check`. > > > Thanks, > > Jason Dusek > >
