You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jojy Varghese <jo...@mesosphere.io> on 2015/12/11 21:32:05 UTC
Re: Review Request 39456: Documentation: added containerizer internals
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39456/
-----------------------------------------------------------
(Updated Dec. 11, 2015, 8:32 p.m.)
Review request for Jie Yu and Timothy Chen.
Changes
-------
added more documentation; sections for future
Repository: mesos
Description
-------
Documentation: added containerizer internals
Diffs (updated)
-----
docs/containerizer-internals.md PRE-CREATION
Diff: https://reviews.apache.org/r/39456/diff/
Testing
-------
Thanks,
Jojy Varghese
Re: Review Request 39456: Documentation: added containerizer internals
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39456/#review110681
-----------------------------------------------------------
Overall nit: add backticks around code/syscalls :)
docs/containerizer-internals.md (line 9)
<https://reviews.apache.org/r/39456/#comment170718>
Note: Containerizers handle executors primarily. The only tasks the containerizers have control over are command tasks (launched by the executors included with Mesos).
docs/containerizer-internals.md (line 10)
<https://reviews.apache.org/r/39456/#comment170719>
Nit: Oxford comma.
docs/containerizer-internals.md (lines 18 - 19)
<https://reviews.apache.org/r/39456/#comment170720>
s/"containerizers"/`--containerizers`/
Ditto for all flags in the file.
docs/containerizer-internals.md (lines 21 - 23)
<https://reviews.apache.org/r/39456/#comment170721>
Nit: reword "will generate an executor" to "will use a default executor".
Nit: Can you surround `TaskInfo`, `mesos-executor`, and `mesos-docker-executor` with backticks?
docs/containerizer-internals.md (line 24)
<https://reviews.apache.org/r/39456/#comment170722>
What are the potential changes MESOS-1718 could have?
docs/containerizer-internals.md (line 75)
<https://reviews.apache.org/r/39456/#comment170725>
Doesn't it also fetch all the URIs?
docs/containerizer-internals.md (lines 94 - 97)
<https://reviews.apache.org/r/39456/#comment170728>
Nit: periods at the ends.
docs/containerizer-internals.md (line 105)
<https://reviews.apache.org/r/39456/#comment170729>
Nit: Oxford comma
- Joseph Wu
On Dec. 15, 2015, 10:22 a.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39456/
> -----------------------------------------------------------
>
> (Updated Dec. 15, 2015, 10:22 a.m.)
>
>
> Review request for mesos, Jie Yu and Timothy Chen.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Documentation: added containerizer internals
>
>
> Diffs
> -----
>
> docs/containerizer-internals.md PRE-CREATION
> docs/home.md a01612ec8347eb8a7a9277b829365b6c3a1fe9e3
>
> Diff: https://reviews.apache.org/r/39456/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 39456: Documentation: added containerizer internals
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39456/#review110764
-----------------------------------------------------------
Ship it!
docs/containerizer-internals.md (lines 54 - 55)
<https://reviews.apache.org/r/39456/#comment170767>
Do you need to remove the second half of this sentense?
- Jie Yu
On Dec. 16, 2015, 9:48 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39456/
> -----------------------------------------------------------
>
> (Updated Dec. 16, 2015, 9:48 p.m.)
>
>
> Review request for mesos, Jie Yu and Timothy Chen.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Documentation: added containerizer internals
>
>
> Diffs
> -----
>
> docs/containerizer-internals.md PRE-CREATION
> docs/home.md a01612ec8347eb8a7a9277b829365b6c3a1fe9e3
>
> Diff: https://reviews.apache.org/r/39456/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 39456: Documentation: added containerizer internals
Posted by Jojy Varghese <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39456/
-----------------------------------------------------------
(Updated Dec. 16, 2015, 9:48 p.m.)
Review request for mesos, Jie Yu and Timothy Chen.
Changes
-------
addressed reviews.
Repository: mesos
Description
-------
Documentation: added containerizer internals
Diffs (updated)
-----
docs/containerizer-internals.md PRE-CREATION
docs/home.md a01612ec8347eb8a7a9277b829365b6c3a1fe9e3
Diff: https://reviews.apache.org/r/39456/diff/
Testing
-------
Thanks,
Jojy Varghese
Re: Review Request 39456: Documentation: added containerizer internals
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39456/#review110581
-----------------------------------------------------------
docs/containerizer-internals.md (lines 39 - 43)
<https://reviews.apache.org/r/39456/#comment170527>
This is not true. Composing containerizer will compose the specified containerizers together and act like a single containerizer.
docs/containerizer-internals.md (lines 55 - 61)
<https://reviews.apache.org/r/39456/#comment170528>
I would mention two cases here:
1) User specifies a TaskInfo without an executor
2) User specifies a TaskInfo with an executor
The flow of the above 2) is different. Please read the code to understand the difference.
docs/containerizer-internals.md (line 72)
<https://reviews.apache.org/r/39456/#comment170546>
For the executor using Launcher (link to the following section).
docs/containerizer-internals.md (line 82)
<https://reviews.apache.org/r/39456/#comment170548>
I would just add a short summary about Launcher:
```
Launcher is responsible for forking/destroying containers.
```
docs/containerizer-internals.md (lines 83 - 87)
<https://reviews.apache.org/r/39456/#comment170549>
I would remove this.
docs/containerizer-internals.md (lines 86 - 87)
<https://reviews.apache.org/r/39456/#comment170547>
User is not allowed to set the 'setup' function.
docs/containerizer-internals.md (line 94)
<https://reviews.apache.org/r/39456/#comment170550>
using 'clone' system call
- Jie Yu
On Dec. 15, 2015, 6:22 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39456/
> -----------------------------------------------------------
>
> (Updated Dec. 15, 2015, 6:22 p.m.)
>
>
> Review request for mesos, Jie Yu and Timothy Chen.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Documentation: added containerizer internals
>
>
> Diffs
> -----
>
> docs/containerizer-internals.md PRE-CREATION
> docs/home.md a01612ec8347eb8a7a9277b829365b6c3a1fe9e3
>
> Diff: https://reviews.apache.org/r/39456/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 39456: Documentation: added containerizer internals
Posted by Jojy Varghese <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39456/
-----------------------------------------------------------
(Updated Dec. 15, 2015, 6:22 p.m.)
Review request for mesos, Jie Yu and Timothy Chen.
Changes
-------
addressed review comments.
Repository: mesos
Description
-------
Documentation: added containerizer internals
Diffs (updated)
-----
docs/containerizer-internals.md PRE-CREATION
docs/home.md a01612ec8347eb8a7a9277b829365b6c3a1fe9e3
Diff: https://reviews.apache.org/r/39456/diff/
Testing
-------
Thanks,
Jojy Varghese
Re: Review Request 39456: Documentation: added containerizer internals
Posted by Jojy Varghese <jo...@mesosphere.io>.
> On Dec. 15, 2015, 12:35 a.m., Guangya Liu wrote:
> > docs/containerizer-internals.md, lines 15-23
> > <https://reviews.apache.org/r/39456/diff/4/?file=1163799#file1163799line15>
> >
> > What about moveing this after ### Type of containerizers
>
> Jojy Varghese wrote:
> Since this section is common to all containerizers, I liked it here. WDYT?
>
> Thanks Guangya.
>
> Guangya Liu wrote:
> It is ok to put it here, my thinking is that it would be better if we put this after giving end user a concept introduciton for all of the containerizers. I do not have strong opinion on this and this depends on you ;-)
dropping this as per our comments.
- Jojy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39456/#review110340
-----------------------------------------------------------
On Dec. 15, 2015, 6:22 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39456/
> -----------------------------------------------------------
>
> (Updated Dec. 15, 2015, 6:22 p.m.)
>
>
> Review request for mesos, Jie Yu and Timothy Chen.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Documentation: added containerizer internals
>
>
> Diffs
> -----
>
> docs/containerizer-internals.md PRE-CREATION
> docs/home.md a01612ec8347eb8a7a9277b829365b6c3a1fe9e3
>
> Diff: https://reviews.apache.org/r/39456/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 39456: Documentation: added containerizer internals
Posted by Jojy Varghese <jo...@mesosphere.io>.
> On Dec. 15, 2015, 12:35 a.m., Guangya Liu wrote:
> > docs/containerizer-internals.md, lines 6-10
> > <https://reviews.apache.org/r/39456/diff/4/?file=1163799#file1163799line6>
> >
> > Does this still needed? As this was already mentioned in https://github.com/apache/mesos/blob/master/docs/containerizer.md#mesos-containerizer
>
> Jojy Varghese wrote:
> This was added as per review comments from other reviewers. The idea was to add a little note about what containerizers are.
dropping this as per the comments. Thanks again.
- Jojy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39456/#review110340
-----------------------------------------------------------
On Dec. 15, 2015, 6:22 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39456/
> -----------------------------------------------------------
>
> (Updated Dec. 15, 2015, 6:22 p.m.)
>
>
> Review request for mesos, Jie Yu and Timothy Chen.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Documentation: added containerizer internals
>
>
> Diffs
> -----
>
> docs/containerizer-internals.md PRE-CREATION
> docs/home.md a01612ec8347eb8a7a9277b829365b6c3a1fe9e3
>
> Diff: https://reviews.apache.org/r/39456/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 39456: Documentation: added containerizer internals
Posted by Jojy Varghese <jo...@mesosphere.io>.
> On Dec. 15, 2015, 12:35 a.m., Guangya Liu wrote:
> > docs/containerizer-internals.md, lines 15-23
> > <https://reviews.apache.org/r/39456/diff/4/?file=1163799#file1163799line15>
> >
> > What about moveing this after ### Type of containerizers
Since this section is common to all containerizers, I liked it here. WDYT?
Thanks Guangya.
> On Dec. 15, 2015, 12:35 a.m., Guangya Liu wrote:
> > docs/containerizer-internals.md, lines 21-23
> > <https://reviews.apache.org/r/39456/diff/4/?file=1163799#file1163799line21>
> >
> > Just a kind reminder, https://issues.apache.org/jira/browse/MESOS-1718 is planning to move get exeucutor info from slave to master, after this is fixed, it will be master who will generate an executor.
Thanks again for the reminder. Do you think we should have this in the documentation now or update the documentation when the change goes in?
- Jojy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39456/#review110340
-----------------------------------------------------------
On Dec. 14, 2015, 8:37 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39456/
> -----------------------------------------------------------
>
> (Updated Dec. 14, 2015, 8:37 p.m.)
>
>
> Review request for mesos, Jie Yu and Timothy Chen.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Documentation: added containerizer internals
>
>
> Diffs
> -----
>
> docs/containerizer-internals.md PRE-CREATION
> docs/home.md a01612ec8347eb8a7a9277b829365b6c3a1fe9e3
>
> Diff: https://reviews.apache.org/r/39456/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 39456: Documentation: added containerizer internals
Posted by Guangya Liu <gy...@gmail.com>.
> On Dec. 15, 2015, 12:35 a.m., Guangya Liu wrote:
> > docs/containerizer-internals.md, lines 21-23
> > <https://reviews.apache.org/r/39456/diff/4/?file=1163799#file1163799line21>
> >
> > Just a kind reminder, https://issues.apache.org/jira/browse/MESOS-1718 is planning to move get exeucutor info from slave to master, after this is fixed, it will be master who will generate an executor.
>
> Jojy Varghese wrote:
> Thanks again for the reminder. Do you think we should have this in the documentation now or update the documentation when the change goes in?
I think that adding a TODO here by refering to MESOS-1718 is good enough.
> On Dec. 15, 2015, 12:35 a.m., Guangya Liu wrote:
> > docs/containerizer-internals.md, lines 15-23
> > <https://reviews.apache.org/r/39456/diff/4/?file=1163799#file1163799line15>
> >
> > What about moveing this after ### Type of containerizers
>
> Jojy Varghese wrote:
> Since this section is common to all containerizers, I liked it here. WDYT?
>
> Thanks Guangya.
It is ok to put it here, my thinking is that it would be better if we put this after giving end user a concept introduciton for all of the containerizers. I do not have strong opinion on this and this depends on you ;-)
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39456/#review110340
-----------------------------------------------------------
On Dec. 14, 2015, 8:37 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39456/
> -----------------------------------------------------------
>
> (Updated Dec. 14, 2015, 8:37 p.m.)
>
>
> Review request for mesos, Jie Yu and Timothy Chen.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Documentation: added containerizer internals
>
>
> Diffs
> -----
>
> docs/containerizer-internals.md PRE-CREATION
> docs/home.md a01612ec8347eb8a7a9277b829365b6c3a1fe9e3
>
> Diff: https://reviews.apache.org/r/39456/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 39456: Documentation: added containerizer internals
Posted by Jojy Varghese <jo...@mesosphere.io>.
> On Dec. 15, 2015, 12:35 a.m., Guangya Liu wrote:
> > docs/containerizer-internals.md, lines 6-10
> > <https://reviews.apache.org/r/39456/diff/4/?file=1163799#file1163799line6>
> >
> > Does this still needed? As this was already mentioned in https://github.com/apache/mesos/blob/master/docs/containerizer.md#mesos-containerizer
This was added as per review comments from other reviewers. The idea was to add a little note about what containerizers are.
- Jojy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39456/#review110340
-----------------------------------------------------------
On Dec. 15, 2015, 6:22 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39456/
> -----------------------------------------------------------
>
> (Updated Dec. 15, 2015, 6:22 p.m.)
>
>
> Review request for mesos, Jie Yu and Timothy Chen.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Documentation: added containerizer internals
>
>
> Diffs
> -----
>
> docs/containerizer-internals.md PRE-CREATION
> docs/home.md a01612ec8347eb8a7a9277b829365b6c3a1fe9e3
>
> Diff: https://reviews.apache.org/r/39456/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 39456: Documentation: added containerizer internals
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39456/#review110340
-----------------------------------------------------------
docs/containerizer-internals.md (lines 6 - 10)
<https://reviews.apache.org/r/39456/#comment170152>
Does this still needed? As this was already mentioned in https://github.com/apache/mesos/blob/master/docs/containerizer.md#mesos-containerizer
docs/containerizer-internals.md (line 10)
<https://reviews.apache.org/r/39456/#comment170150>
s/eg/eg,
docs/containerizer-internals.md (lines 15 - 23)
<https://reviews.apache.org/r/39456/#comment170163>
What about moveing this after ### Type of containerizers
docs/containerizer-internals.md (line 18)
<https://reviews.apache.org/r/39456/#comment170153>
s/eg./eg,
docs/containerizer-internals.md (lines 21 - 23)
<https://reviews.apache.org/r/39456/#comment170154>
Just a kind reminder, https://issues.apache.org/jira/browse/MESOS-1718 is planning to move get exeucutor info from slave to master, after this is fixed, it will be master who will generate an executor.
- Guangya Liu
On Dec. 14, 2015, 8:37 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39456/
> -----------------------------------------------------------
>
> (Updated Dec. 14, 2015, 8:37 p.m.)
>
>
> Review request for mesos, Jie Yu and Timothy Chen.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Documentation: added containerizer internals
>
>
> Diffs
> -----
>
> docs/containerizer-internals.md PRE-CREATION
> docs/home.md a01612ec8347eb8a7a9277b829365b6c3a1fe9e3
>
> Diff: https://reviews.apache.org/r/39456/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 39456: Documentation: added containerizer internals
Posted by Jojy Varghese <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39456/
-----------------------------------------------------------
(Updated Dec. 14, 2015, 8:37 p.m.)
Review request for mesos, Jie Yu and Timothy Chen.
Changes
-------
post review changes.
Repository: mesos
Description
-------
Documentation: added containerizer internals
Diffs (updated)
-----
docs/containerizer-internals.md PRE-CREATION
docs/home.md a01612ec8347eb8a7a9277b829365b6c3a1fe9e3
Diff: https://reviews.apache.org/r/39456/diff/
Testing
-------
Thanks,
Jojy Varghese