You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Till Toenshoff <to...@me.com> on 2014/04/09 02:25:39 UTC
Review Request 20141: Added containerizer.proto.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20141/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman and Niklas Nielsen.
Repository: mesos-git
Description
-------
Adds containerizer.proto containing protobuf message definitions for "launch" and "update". Both are used in the upcoming External Containerizer.
These protos are defined within their own namespace: containerizer.
Diffs
-----
src/Makefile.am 95f133d
src/slave/containerizer/containerizer.proto PRE-CREATION
Diff: https://reviews.apache.org/r/20141/diff/
Testing
-------
make check
Thanks,
Till Toenshoff
Re: Review Request 20141: Added containerizer.proto.
Posted by Till Toenshoff <to...@me.com>.
> On April 9, 2014, 8:05 p.m., Vinod Kone wrote:
> > src/Makefile.am, line 110
> > <https://reviews.apache.org/r/20141/diff/2/?file=553536#file553536line110>
> >
> > Do we want the containerizer proto in a separate python module? I don't know what is idiomatic in Python but can we just add containerizer protos to mesos_pb2? Similarly for Java.
> >
> >
AFAIK .proto-files do only support a single namespace per definition file. I dont see how I could get both requirements satisfied; having a separate namespace (containerizer::) and having a single output file (mesos_pb2.py).
> On April 9, 2014, 8:05 p.m., Vinod Kone wrote:
> > src/Makefile.am, lines 154-157
> > <https://reviews.apache.org/r/20141/diff/2/?file=553536#file553536line154>
> >
> > this seems to be the same rule as above?
> On April 9, 2014, 8:05 p.m., Vinod Kone wrote:
> > src/slave/containerizer/containerizer.proto, lines 24-46
> > <https://reviews.apache.org/r/20141/diff/2/?file=553537#file553537line24>
> >
> > What about protobufs for Usage, Wait and Destroy?
> >
> > Should Termination also be pulled into this namespace?
The parameter definitions for the containerizer API on 'usage', 'wait' and 'destroy' do currently only contain a single parameter; ContainerID (see mesos_containerizer.hpp:68-74), hence I thought that their explicit definitions would not add any value.
For 'Termination', that seems to make sense indeed. Will update accordingly.
- Till
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20141/#review39926
-----------------------------------------------------------
On April 9, 2014, 12:25 a.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20141/
> -----------------------------------------------------------
>
> (Updated April 9, 2014, 12:25 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Niklas Nielsen.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Adds containerizer.proto containing protobuf message definitions for "launch" and "update". Both are used in the upcoming External Containerizer.
>
> These protos are defined within their own namespace: containerizer.
>
>
> Diffs
> -----
>
> src/Makefile.am 95f133d
> src/slave/containerizer/containerizer.proto PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20141/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Till Toenshoff
>
>
Re: Review Request 20141: Added containerizer.proto.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20141/#review39926
-----------------------------------------------------------
src/Makefile.am
<https://reviews.apache.org/r/20141/#comment72693>
Do we want the containerizer proto in a separate python module? I don't know what is idiomatic in Python but can we just add containerizer protos to mesos_pb2? Similarly for Java.
src/Makefile.am
<https://reviews.apache.org/r/20141/#comment72691>
this seems to be the same rule as above?
src/slave/containerizer/containerizer.proto
<https://reviews.apache.org/r/20141/#comment72688>
What about protobufs for Usage, Wait and Destroy?
Should Termination also be pulled into this namespace?
- Vinod Kone
On April 9, 2014, 12:25 a.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20141/
> -----------------------------------------------------------
>
> (Updated April 9, 2014, 12:25 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Niklas Nielsen.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Adds containerizer.proto containing protobuf message definitions for "launch" and "update". Both are used in the upcoming External Containerizer.
>
> These protos are defined within their own namespace: containerizer.
>
>
> Diffs
> -----
>
> src/Makefile.am 95f133d
> src/slave/containerizer/containerizer.proto PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20141/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Till Toenshoff
>
>
Re: Review Request 20141: Added containerizer.proto.
Posted by Till Toenshoff <to...@me.com>.
> On April 10, 2014, 7:04 p.m., Benjamin Hindman wrote:
> > src/slave/containerizer/containerizer.proto, line 1
> > <https://reviews.apache.org/r/20141/diff/2/?file=553537#file553537line1>
> >
> > Let's actually put this file in include/mesos as we want to "install" this for external containerizer programs to use.
That actually also simplifies things, double win :)
- Till
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20141/#review40061
-----------------------------------------------------------
On April 9, 2014, 12:25 a.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20141/
> -----------------------------------------------------------
>
> (Updated April 9, 2014, 12:25 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Niklas Nielsen.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Adds containerizer.proto containing protobuf message definitions for "launch" and "update". Both are used in the upcoming External Containerizer.
>
> These protos are defined within their own namespace: containerizer.
>
>
> Diffs
> -----
>
> src/Makefile.am 95f133d
> src/slave/containerizer/containerizer.proto PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20141/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Till Toenshoff
>
>
Re: Review Request 20141: Added containerizer.proto.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20141/#review40061
-----------------------------------------------------------
src/slave/containerizer/containerizer.proto
<https://reviews.apache.org/r/20141/#comment72867>
Let's actually put this file in include/mesos as we want to "install" this for external containerizer programs to use.
- Benjamin Hindman
On April 9, 2014, 12:25 a.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20141/
> -----------------------------------------------------------
>
> (Updated April 9, 2014, 12:25 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Niklas Nielsen.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Adds containerizer.proto containing protobuf message definitions for "launch" and "update". Both are used in the upcoming External Containerizer.
>
> These protos are defined within their own namespace: containerizer.
>
>
> Diffs
> -----
>
> src/Makefile.am 95f133d
> src/slave/containerizer/containerizer.proto PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20141/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Till Toenshoff
>
>
Re: Review Request 20141: Added containerizer.proto.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20141/#review39859
-----------------------------------------------------------
Patch looks great!
Reviews applied: [20141]
All tests passed.
- Mesos ReviewBot
On April 9, 2014, 12:25 a.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20141/
> -----------------------------------------------------------
>
> (Updated April 9, 2014, 12:25 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Niklas Nielsen.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Adds containerizer.proto containing protobuf message definitions for "launch" and "update". Both are used in the upcoming External Containerizer.
>
> These protos are defined within their own namespace: containerizer.
>
>
> Diffs
> -----
>
> src/Makefile.am 95f133d
> src/slave/containerizer/containerizer.proto PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20141/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Till Toenshoff
>
>
Re: Review Request 20141: Added containerizer.proto.
Posted by Till Toenshoff <to...@me.com>.
> On April 14, 2014, 5:09 a.m., Benjamin Hindman wrote:
> > include/mesos/containerizer.proto, lines 23-24
> > <https://reviews.apache.org/r/20141/diff/3/?file=555024#file555024line23>
> >
> > Sorry Till, I wasn't explicit enough about this refactor. Let's put this file in 'include/mesos/containerizer/containerizer.proto'. Then we can make the Java package match the C++ one, i.e., 'org.apache.mesos.containerizer'. Then the outer classname can be consistent with the others and just be 'Protos'.
Ah, ok. Makes sense.
> On April 14, 2014, 5:09 a.m., Benjamin Hindman wrote:
> > src/Makefile.am, lines 108-110
> > <https://reviews.apache.org/r/20141/diff/3/?file=555025#file555025line108>
> >
> > Now this should be able to be just:
> >
> > JAVA_PROTOS = \
> > java/generated/org/apache/mesos/Protos.java \
> > java/generated/org/apache/mesos/containerizer/Protos.java
> >
> >
> > And then below you should be able to define the Java targets like what was done for C++ and what you did for Python.
I may be missing something but IIUC, then using a subfolder for that containerizer.proto does force me to add more rules like:
[...]
containerizer/%.pb.cc containerizer/%.pb.h: $(CONTAINERIZER_PROTO)
$(MKDIR_P) $(@D)
$(PROTOC) $(PROTOCFLAGS) --cpp_out=. $^
java/generated/org/apache/mesos/Protos.java: $(MESOS_PROTO)
$(MKDIR_P) $(@D)
$(PROTOC) $(PROTOCFLAGS) --java_out=java/generated $^
java/generated/org/apache/mesos/containerizer/Protos.java: $(CONTAINERIZER_PROTO)
$(MKDIR_P) $(@D)
$(PROTOC) $(PROTOCFLAGS) --java_out=java/generated $^
python/src/mesos_pb2.py: $(MESOS_PROTO)
$(MKDIR_P) $(@D)
$(PROTOC) $(PROTOCFLAGS) --python_out=python/src $^
python/src/containerizer/containerizer_pb2.py: $(CONTAINERIZER_PROTO)
$(MKDIR_P) $(@D)
$(PROTOC) $(PROTOCFLAGS) --python_out=python/src $^
I will also have to adapt setup.py to cover subpackages:
setup(name = 'mesos',
version = '0.19.0',
description = 'Mesos',
package_dir = { '': 'src' },
packages = ['.', 'containerizer'],
install_requires = ['protobuf>=2.5.0'],
ext_modules = [mesos_module])
I would be happy to do so, but please confirm that this is what you aim for.
> On April 14, 2014, 5:09 a.m., Benjamin Hindman wrote:
> > src/Makefile.am, lines 214-220
> > <https://reviews.apache.org/r/20141/diff/3/?file=555025#file555025line214>
> >
> > You'll want to update these with the new containerizer stuff too so that it gets installed. And feel free to create a 'containerizer.hpp' which wraps containerizer.pb.h just as we've done with 'mesos.hpp'.
Yes, that makes sense.
> On April 14, 2014, 5:09 a.m., Benjamin Hindman wrote:
> > src/slave/containerizer/containerizer.hpp, line 24
> > <https://reviews.apache.org/r/20141/diff/3/?file=555026#file555026line24>
> >
> > As of now this isn't being installed and the convention is that we only use <> for things that get installed.
> >
> > Also, if you create a wrapper then you can use it here instead, i.e., 'mesos/containerizer/containerizer.hpp' (as we do with mesos/mesos.hpp).
Silly me :) .... should be fixed by introducing the wrapping header which will get installed, so this becomes
#include <mesos/containerizer/containerizer.hpp>
- Till
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20141/#review40228
-----------------------------------------------------------
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
>
>
Re: Review Request 20141: Added containerizer.proto.
Posted by Till Toenshoff <to...@me.com>.
> On April 14, 2014, 5:09 a.m., Benjamin Hindman wrote:
> > src/Makefile.am, lines 108-110
> > <https://reviews.apache.org/r/20141/diff/3/?file=555025#file555025line108>
> >
> > Now this should be able to be just:
> >
> > JAVA_PROTOS = \
> > java/generated/org/apache/mesos/Protos.java \
> > java/generated/org/apache/mesos/containerizer/Protos.java
> >
> >
> > And then below you should be able to define the Java targets like what was done for C++ and what you did for Python.
>
> Till Toenshoff wrote:
> I may be missing something but IIUC, then using a subfolder for that containerizer.proto does force me to add more rules like:
>
> [...]
>
> containerizer/%.pb.cc containerizer/%.pb.h: $(CONTAINERIZER_PROTO)
> $(MKDIR_P) $(@D)
> $(PROTOC) $(PROTOCFLAGS) --cpp_out=. $^
>
> java/generated/org/apache/mesos/Protos.java: $(MESOS_PROTO)
> $(MKDIR_P) $(@D)
> $(PROTOC) $(PROTOCFLAGS) --java_out=java/generated $^
>
> java/generated/org/apache/mesos/containerizer/Protos.java: $(CONTAINERIZER_PROTO)
> $(MKDIR_P) $(@D)
> $(PROTOC) $(PROTOCFLAGS) --java_out=java/generated $^
>
> python/src/mesos_pb2.py: $(MESOS_PROTO)
> $(MKDIR_P) $(@D)
> $(PROTOC) $(PROTOCFLAGS) --python_out=python/src $^
>
> python/src/containerizer/containerizer_pb2.py: $(CONTAINERIZER_PROTO)
> $(MKDIR_P) $(@D)
> $(PROTOC) $(PROTOCFLAGS) --python_out=python/src $^
>
>
> I will also have to adapt setup.py to cover subpackages:
>
> setup(name = 'mesos',
> version = '0.19.0',
> description = 'Mesos',
> package_dir = { '': 'src' },
> packages = ['.', 'containerizer'],
> install_requires = ['protobuf>=2.5.0'],
> ext_modules = [mesos_module])
>
> I would be happy to do so, but please confirm that this is what you aim for.
I ended up doing it differently as I had trouble getting the subpackage stuff to work properly.
- Till
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20141/#review40228
-----------------------------------------------------------
On April 18, 2014, 3:20 a.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20141/
> -----------------------------------------------------------
>
> (Updated April 18, 2014, 3:20 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/containerizer.hpp PRE-CREATION
> include/mesos/containerizer/containerizer.proto PRE-CREATION
> src/Makefile.am 560b4c7
> src/slave/containerizer/containerizer.hpp d9ae326
> src/slave/containerizer/mesos_containerizer.hpp ee1fd30
> src/slave/containerizer/mesos_containerizer.cpp 1ce41d7
> src/slave/slave.hpp 1e98795
> src/slave/slave.cpp d6ec87c
> src/tests/containerizer.hpp a9f1531
> src/tests/containerizer.cpp bfb9341
>
> Diff: https://reviews.apache.org/r/20141/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Till Toenshoff
>
>
Re: Review Request 20141: Added containerizer.proto.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20141/#review40228
-----------------------------------------------------------
Looks good Till, just a little more code motion with the containerizer.proto and I think we'll be good to go!
include/mesos/containerizer.proto
<https://reviews.apache.org/r/20141/#comment73165>
Sorry Till, I wasn't explicit enough about this refactor. Let's put this file in 'include/mesos/containerizer/containerizer.proto'. Then we can make the Java package match the C++ one, i.e., 'org.apache.mesos.containerizer'. Then the outer classname can be consistent with the others and just be 'Protos'.
src/Makefile.am
<https://reviews.apache.org/r/20141/#comment73166>
Now this should be able to be just:
JAVA_PROTOS = \
java/generated/org/apache/mesos/Protos.java \
java/generated/org/apache/mesos/containerizer/Protos.java
And then below you should be able to define the Java targets like what was done for C++ and what you did for Python.
src/Makefile.am
<https://reviews.apache.org/r/20141/#comment73167>
You'll want to update these with the new containerizer stuff too so that it gets installed. And feel free to create a 'containerizer.hpp' which wraps containerizer.pb.h just as we've done with 'mesos.hpp'.
src/slave/containerizer/containerizer.hpp
<https://reviews.apache.org/r/20141/#comment73191>
As of now this isn't being installed and the convention is that we only use <> for things that get installed.
Also, if you create a wrapper then you can use it here instead, i.e., 'mesos/containerizer/containerizer.hpp' (as we do with mesos/mesos.hpp).
- Benjamin Hindman
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
>
>
Re: Review Request 20141: Added containerizer.proto.
Posted by Adam B <ad...@mesosphere.io>.
> On April 14, 2014, 5:05 p.m., Adam B wrote:
> > Please comment the src/dest of each protobuf message, and don't make so many fields required.
Also, update this and r/17567 so it is clear that r/17567 depends on this instead of the discarded r/19901.
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20141/#review40323
-----------------------------------------------------------
On April 10, 2014, 7:03 p.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20141/
> -----------------------------------------------------------
>
> (Updated April 10, 2014, 7:03 p.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
>
>
Re: Review Request 20141: Added containerizer.proto.
Posted by Till Toenshoff <to...@me.com>.
> 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
>
>
Re: Review Request 20141: Added containerizer.proto.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20141/#review40323
-----------------------------------------------------------
Please comment the src/dest of each protobuf message, and don't make so many fields required.
include/mesos/containerizer.proto
<https://reviews.apache.org/r/20141/#comment73320>
Sent from Mesos slave to external containerizer (e.g. Deimos)? Let's explicitly document the to/from and direction of these messages.
include/mesos/containerizer.proto
<https://reviews.apache.org/r/20141/#comment73308>
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.
include/mesos/containerizer.proto
<https://reviews.apache.org/r/20141/#comment73321>
Is this the external containerizer updating the slave/executor, or vice versa?
include/mesos/containerizer.proto
<https://reviews.apache.org/r/20141/#comment73316>
optional? In case we just want to update other aspects of the container (user, frameworkId, uris, etc.)?
include/mesos/containerizer.proto
<https://reviews.apache.org/r/20141/#comment73322>
Termination message comes from the (external) containerizer to the slave, right?
include/mesos/containerizer.proto
<https://reviews.apache.org/r/20141/#comment73314>
optional?
- Adam B
On April 10, 2014, 7:03 p.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20141/
> -----------------------------------------------------------
>
> (Updated April 10, 2014, 7:03 p.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
>
>
Re: Review Request 20141: Added containerizer.proto.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20141/#review40738
-----------------------------------------------------------
Ship it!
I'm getting this committed. I've made some very minor edits here (used '$(top_srcdir)' instead of '..'). Also, I'm assuming we'll be adding (and possibly tweaking) the protobufs in containerizer.proto as we solidify the updates to the Containerizer that Niklas is working on.
- Benjamin Hindman
On April 18, 2014, 3:20 a.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20141/
> -----------------------------------------------------------
>
> (Updated April 18, 2014, 3:20 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/containerizer.hpp PRE-CREATION
> include/mesos/containerizer/containerizer.proto PRE-CREATION
> src/Makefile.am 560b4c7
> src/slave/containerizer/containerizer.hpp d9ae326
> src/slave/containerizer/mesos_containerizer.hpp ee1fd30
> src/slave/containerizer/mesos_containerizer.cpp 1ce41d7
> src/slave/slave.hpp 1e98795
> src/slave/slave.cpp d6ec87c
> src/tests/containerizer.hpp a9f1531
> src/tests/containerizer.cpp bfb9341
>
> Diff: https://reviews.apache.org/r/20141/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Till Toenshoff
>
>
Re: Review Request 20141: Added containerizer.proto.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20141/#review40740
-----------------------------------------------------------
Bad patch!
Reviews applied: [20141]
Failed command: make -j3 distcheck GTEST_FILTER='' >/dev/null
Error:
configure: WARNING: can not find python-boto
-------------------------------------------------------------------
mesos-ec2 services will not function.
-------------------------------------------------------------------
ev.c:1531:31: warning: 'ev_default_loop_ptr' initialized and declared 'extern' [enabled by default]
ev.c: In function 'evpipe_write':
ev.c:2160:17: warning: ignoring return value of 'write', declared with attribute warn_unused_result [-Wunused-result]
ev.c:2172:17: warning: ignoring return value of 'write', declared with attribute warn_unused_result [-Wunused-result]
ev.c: In function 'pipecb':
ev.c:2193:16: warning: ignoring return value of 'read', declared with attribute warn_unused_result [-Wunused-result]
ev.c:2207:16: warning: ignoring return value of 'read', declared with attribute warn_unused_result [-Wunused-result]
In file included from /usr/include/c++/4.6/ext/hash_set:61:0,
from src/glog/stl_logging.h:54,
from src/stl_logging_unittest.cc:34:
/usr/include/c++/4.6/backward/backward_warning.h:33:2: warning: #warning This file includes at least one deprecated or antiquated header which may be removed without further notice at a future date. Please use a non-deprecated interface with equivalent functionality instead. For a listing of replacement headers and interfaces, consult the file backward_warning.h. To disable this warning use -Wno-deprecated. [-Wcpp]
In file included from src/utilities.h:73:0,
from src/googletest.h:38,
from src/stl_logging_unittest.cc:48:
src/base/mutex.h:137:0: warning: "_XOPEN_SOURCE" redefined [enabled by default]
/usr/include/features.h:166:0: note: this is the location of the previous definition
warning: no files found matching 'Makefile' under directory 'docs'
warning: no files found matching 'indexsidebar.html' under directory 'docs'
ar: creating libleveldb.a
In file included from ../../src/slave/slave.hpp:51:0,
from ../../src/local/main.cpp:32:
../../src/slave/containerizer/containerizer.hpp:27:49: fatal error: mesos/containerizer/containerizer.hpp: No such file or directory
compilation terminated.
make[3]: *** [local/mesos_local-main.o] Error 1
make[3]: *** Waiting for unfinished jobs....
make[2]: *** [all] Error 2
make[1]: *** [all-recursive] Error 1
make: *** [distcheck] Error 1
- Mesos ReviewBot
On April 18, 2014, 3:20 a.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20141/
> -----------------------------------------------------------
>
> (Updated April 18, 2014, 3:20 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/containerizer.hpp PRE-CREATION
> include/mesos/containerizer/containerizer.proto PRE-CREATION
> src/Makefile.am 560b4c7
> src/slave/containerizer/containerizer.hpp d9ae326
> src/slave/containerizer/mesos_containerizer.hpp ee1fd30
> src/slave/containerizer/mesos_containerizer.cpp 1ce41d7
> src/slave/slave.hpp 1e98795
> src/slave/slave.cpp d6ec87c
> src/tests/containerizer.hpp a9f1531
> src/tests/containerizer.cpp bfb9341
>
> Diff: https://reviews.apache.org/r/20141/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Till Toenshoff
>
>
Re: Review Request 20141: Added containerizer.proto.
Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20141/
-----------------------------------------------------------
(Updated April 18, 2014, 3:20 a.m.)
Review request for mesos, Benjamin Hindman and Niklas Nielsen.
Changes
-------
- Moved containerizer.proto into mesos/include/containerizer
- Reduced amount of required attributes within the Launch message
- Added protobuf results wrapping containerizer.hpp as an installed header
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 (updated)
-----
include/mesos/containerizer/containerizer.hpp PRE-CREATION
include/mesos/containerizer/containerizer.proto PRE-CREATION
src/Makefile.am 560b4c7
src/slave/containerizer/containerizer.hpp d9ae326
src/slave/containerizer/mesos_containerizer.hpp ee1fd30
src/slave/containerizer/mesos_containerizer.cpp 1ce41d7
src/slave/slave.hpp 1e98795
src/slave/slave.cpp d6ec87c
src/tests/containerizer.hpp a9f1531
src/tests/containerizer.cpp bfb9341
Diff: https://reviews.apache.org/r/20141/diff/
Testing
-------
make check
Thanks,
Till Toenshoff
Re: Review Request 20141: Added containerizer.proto.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20141/#review40119
-----------------------------------------------------------
Patch looks great!
Reviews applied: [20141]
All tests passed.
- Mesos ReviewBot
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
>
>
Re: Review Request 20141: Added containerizer.proto.
Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
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.
Changes
-------
Moved Termination to containerizer.proto
Added containerizer.proto to Java builds
Moved containerizer.proto into include/mesos
Removed duplicate Java rule
Repository: mesos-git
Description (updated)
-------
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 (updated)
-----
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