> On April 15, 2014, 12:05 a.m., Adam B wrote: > > Please comment the src/dest of each protobuf message, and don't make so > > many fields required. > > Adam B wrote: > Also, update this and r/17567 so it is clear that r/17567 depends on this > instead of the discarded r/19901.
Yes, 17567 was idling until all dependent patches got in a shape that appeared to be close to a commit. > On April 15, 2014, 12:05 a.m., Adam B wrote: > > include/mesos/containerizer.proto, line 53 > > <https://reviews.apache.org/r/20141/diff/3/?file=555024#file555024line53> > > > > Termination message comes from the (external) containerizer to the > > slave, right? Correct, as per above, will enhance the comments on that a bit. > On April 15, 2014, 12:05 a.m., Adam B wrote: > > include/mesos/containerizer.proto, lines 31-38 > > <https://reviews.apache.org/r/20141/diff/3/?file=555024#file555024line31> > > > > Are all of these fields always going to be required by every external > > containerizer until the end of time? As the protobuf docs say, "Required is > > forever." It's a lot safer to use optional just in case anybody ever wants > > to change the type, deprecate the field, or make it repeated. > > Figure out what the 1 or 2 absolutely-required, always-singular fields > > are (ContainerID, FrameworkID?) and make everything else optional. That indeed is a good point. I think it makes sense requiring ContainerID and TaskInfo, rest optional. > On April 15, 2014, 12:05 a.m., Adam B wrote: > > include/mesos/containerizer.proto, line 48 > > <https://reviews.apache.org/r/20141/diff/3/?file=555024#file555024line48> > > > > optional? In case we just want to update other aspects of the container > > (user, frameworkId, uris, etc.)? 'update' is exclusively meant for updating the resource constraints. > On April 15, 2014, 12:05 a.m., Adam B wrote: > > include/mesos/containerizer.proto, lines 58-59 > > <https://reviews.apache.org/r/20141/diff/3/?file=555024#file555024line58> > > > > optional? This is a straight "translation" of the original Containerizer::Termination message which did not put an Option<> around message, hence it is required as per that "translation". We might discuss if that message was always required or not but I would prefer not changing those attributes in this RR to reduce the amount of changes to a needed minimum. > On April 15, 2014, 12:05 a.m., Adam B wrote: > > include/mesos/containerizer.proto, line 44 > > <https://reviews.apache.org/r/20141/diff/3/?file=555024#file555024line44> > > > > Is this the external containerizer updating the slave/executor, or vice > > versa? Will update that comment. > On April 15, 2014, 12:05 a.m., Adam B wrote: > > include/mesos/containerizer.proto, line 28 > > <https://reviews.apache.org/r/20141/diff/3/?file=555024#file555024line28> > > > > Sent from Mesos slave to external containerizer (e.g. Deimos)? Let's > > explicitly document the to/from and direction of these messages. Will update that comment. - Till ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20141/#review40323 ----------------------------------------------------------- On April 11, 2014, 2:03 a.m., Till Toenshoff wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/20141/ > ----------------------------------------------------------- > > (Updated April 11, 2014, 2:03 a.m.) > > > Review request for mesos, Benjamin Hindman and Niklas Nielsen. > > > Repository: mesos-git > > > Description > ------- > > Adds containerizer.proto containing protobuf message definitions for > "Launch", "Update" and "Termination". All of those are used in the upcoming > External Containerizer. > These protos are defined within their own C++ namespace: "containerizer". > > Replaced slave::Containerizer::Termination with the > containerizer::Termination protobuf, gaining a serializable Termination > message. > > The protoc results are added towards the Python Egg (containerizer_pb2.py) > and the Java package (ContainerizerProtos.java). > > > Diffs > ----- > > include/mesos/containerizer.proto PRE-CREATION > src/Makefile.am 95f133d > src/slave/containerizer/containerizer.hpp d9ae326 > src/slave/containerizer/mesos_containerizer.hpp ee1fd30 > src/slave/containerizer/mesos_containerizer.cpp 1ce41d7 > src/slave/slave.hpp 08f6005 > src/slave/slave.cpp cddb241 > src/tests/containerizer.hpp a9f1531 > src/tests/containerizer.cpp bfb9341 > > Diff: https://reviews.apache.org/r/20141/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Till Toenshoff > >
