You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Vinod Kone <vi...@gmail.com> on 2015/08/03 21:26:44 UTC
Review Request 37046: Merged registerFramework() and
reregisterFramework().
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37046/
-----------------------------------------------------------
Review request for mesos, Anand Mazumdar and Ben Mahler.
Bugs: MESOS-3182
https://issues.apache.org/jira/browse/MESOS-3182
Repository: mesos
Description
-------
Merged registerFramework() and reregisterFramework() (and their corresponding continuations) into subscribe() (and _subscribe()).
Diffs
-----
src/master/master.hpp ea18c4e0bb0743747401b9cd5ea14ae9b56ae3cc
src/master/master.cpp 351a3c2b5f551ad065682cea601d2436258e4544
Diff: https://reviews.apache.org/r/37046/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request 37046: Merged registerFramework() and
reregisterFramework().
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37046/#review94028
-----------------------------------------------------------
Patch looks great!
Reviews applied: [37046]
All tests passed.
- Mesos ReviewBot
On Aug. 3, 2015, 9:47 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37046/
> -----------------------------------------------------------
>
> (Updated Aug. 3, 2015, 9:47 p.m.)
>
>
> Review request for mesos, Anand Mazumdar and Ben Mahler.
>
>
> Bugs: MESOS-3182
> https://issues.apache.org/jira/browse/MESOS-3182
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Merged registerFramework() and reregisterFramework() (and their corresponding continuations) into subscribe() (and _subscribe()).
>
>
> Diffs
> -----
>
> src/master/master.hpp ea18c4e0bb0743747401b9cd5ea14ae9b56ae3cc
> src/master/master.cpp 351a3c2b5f551ad065682cea601d2436258e4544
>
> Diff: https://reviews.apache.org/r/37046/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 37046: Merged registerFramework() and
reregisterFramework().
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37046/
-----------------------------------------------------------
(Updated Aug. 3, 2015, 9:47 p.m.)
Review request for mesos, Anand Mazumdar and Ben Mahler.
Changes
-------
benm's. NNFR.
Bugs: MESOS-3182
https://issues.apache.org/jira/browse/MESOS-3182
Repository: mesos
Description
-------
Merged registerFramework() and reregisterFramework() (and their corresponding continuations) into subscribe() (and _subscribe()).
Diffs (updated)
-----
src/master/master.hpp ea18c4e0bb0743747401b9cd5ea14ae9b56ae3cc
src/master/master.cpp 351a3c2b5f551ad065682cea601d2436258e4544
Diff: https://reviews.apache.org/r/37046/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request 37046: Merged registerFramework() and
reregisterFramework().
Posted by Vinod Kone <vi...@gmail.com>.
> On Aug. 3, 2015, 8:07 p.m., Ben Mahler wrote:
> > src/master/master.cpp, lines 1719-1721
> > <https://reviews.apache.org/r/37046/diff/1/?file=1027863#file1027863line1719>
> >
> > Hm.. for later, I suppose we'll want to have a metric helper for calls inside receive. We currently aren't counting the number of call messages, right?
Filed https://issues.apache.org/jira/browse/MESOS-3195
> On Aug. 3, 2015, 8:07 p.m., Ben Mahler wrote:
> > src/master/master.cpp, line 1729
> > <https://reviews.apache.org/r/37046/diff/1/?file=1027863#file1027863line1729>
> >
> > s/registration/subcribe/ ?
fixed.
> On Aug. 3, 2015, 8:07 p.m., Ben Mahler wrote:
> > src/master/master.cpp, lines 1742-1745
> > <https://reviews.apache.org/r/37046/diff/1/?file=1027863#file1027863line1742>
> >
> > Hm.. is the idea that we can remove this because we trust that the scheduler driver will not set this for a register message?
yes. but thinking more, i'll have these checks inside (re-)registerFramework() instead of killing them.
> On Aug. 3, 2015, 8:07 p.m., Ben Mahler wrote:
> > src/master/master.cpp, lines 1754-1755
> > <https://reviews.apache.org/r/37046/diff/1/?file=1027863#file1027863line1754>
> >
> > Hm.. if you're going to combine these mind at least wrapping so that the root checks are together?
> >
> > ```
> > if (validationError.isNone() &&
> > frameworkInfo.user() == "root" && !flags.root_submissions) {
> > }
> > ```
done.
> On Aug. 3, 2015, 8:07 p.m., Ben Mahler wrote:
> > src/master/master.cpp, line 1756
> > <https://reviews.apache.org/r/37046/diff/1/?file=1027863#file1027863line1756>
> >
> > Indentation?
ty.
> On Aug. 3, 2015, 8:07 p.m., Ben Mahler wrote:
> > src/master/master.cpp, lines 1854-1857
> > <https://reviews.apache.org/r/37046/diff/1/?file=1027863#file1027863line1854>
> >
> > Do we want to log differently based on whether the framework id is set?
done.
> On Aug. 3, 2015, 8:07 p.m., Ben Mahler wrote:
> > src/master/master.cpp, line 2044
> > <https://reviews.apache.org/r/37046/diff/1/?file=1027863#file1027863line2044>
> >
> > s/failover/force/ ?
done.
> On Aug. 3, 2015, 8:07 p.m., Ben Mahler wrote:
> > src/master/master.cpp, line 2058
> > <https://reviews.apache.org/r/37046/diff/1/?file=1027863#file1027863line2058>
> >
> > subscription?
done.
> On Aug. 3, 2015, 8:07 p.m., Ben Mahler wrote:
> > src/master/master.cpp, line 2077
> > <https://reviews.apache.org/r/37046/diff/1/?file=1027863#file1027863line2077>
> >
> > Just an observation, it seems not that intuitive that we call failoverFramework when the pids are the same, might benefit from a comment.
i think this was originally done in the days when the driver pid wasn't unique. now that it is unique, we can simplify this a bit. i'll add a TODO for now.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37046/#review93955
-----------------------------------------------------------
On Aug. 3, 2015, 7:26 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37046/
> -----------------------------------------------------------
>
> (Updated Aug. 3, 2015, 7:26 p.m.)
>
>
> Review request for mesos, Anand Mazumdar and Ben Mahler.
>
>
> Bugs: MESOS-3182
> https://issues.apache.org/jira/browse/MESOS-3182
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Merged registerFramework() and reregisterFramework() (and their corresponding continuations) into subscribe() (and _subscribe()).
>
>
> Diffs
> -----
>
> src/master/master.hpp ea18c4e0bb0743747401b9cd5ea14ae9b56ae3cc
> src/master/master.cpp 351a3c2b5f551ad065682cea601d2436258e4544
>
> Diff: https://reviews.apache.org/r/37046/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 37046: Merged registerFramework() and
reregisterFramework().
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37046/#review93955
-----------------------------------------------------------
Ship it!
src/master/master.cpp (lines 1719 - 1721)
<https://reviews.apache.org/r/37046/#comment148400>
Hm.. for later, I suppose we'll want to have a metric helper for calls inside receive. We currently aren't counting the number of call messages, right?
src/master/master.cpp (line 1729)
<https://reviews.apache.org/r/37046/#comment148398>
s/registration/subcribe/ ?
src/master/master.cpp
<https://reviews.apache.org/r/37046/#comment148394>
Hm.. is the idea that we can remove this because we trust that the scheduler driver will not set this for a register message?
src/master/master.cpp (lines 1749 - 1750)
<https://reviews.apache.org/r/37046/#comment148395>
Hm.. if you're going to combine these mind at least wrapping so that the root checks are together?
```
if (validationError.isNone() &&
frameworkInfo.user() == "root" && !flags.root_submissions) {
}
```
src/master/master.cpp (line 1751)
<https://reviews.apache.org/r/37046/#comment148396>
Indentation?
src/master/master.cpp (line 1761)
<https://reviews.apache.org/r/37046/#comment148397>
Mind adding a newline comment between these? I find it pretty dense to read when a TODO is right up against the rest of the comment:
```
// This could happen if a framework tries to subscribe after
// its failover timeout has elapsed or it unregistered itself
// by calling 'stop()' on the scheduler driver.
//
// TODO(vinod): Master should persist admitted frameworks to the
// registry and remove them from it after failover timeout.
```
src/master/master.cpp (lines 1849 - 1852)
<https://reviews.apache.org/r/37046/#comment148403>
Do we want to log differently based on whether the framework id is set?
src/master/master.cpp (line 1890)
<https://reviews.apache.org/r/37046/#comment148406>
s/failover/force/ ?
src/master/master.cpp (line 1904)
<https://reviews.apache.org/r/37046/#comment148405>
subscription?
src/master/master.cpp (line 1917)
<https://reviews.apache.org/r/37046/#comment148409>
Just an observation, it seems not that intuitive that we call failoverFramework when the pids are the same, might benefit from a comment.
- Ben Mahler
On Aug. 3, 2015, 7:26 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37046/
> -----------------------------------------------------------
>
> (Updated Aug. 3, 2015, 7:26 p.m.)
>
>
> Review request for mesos, Anand Mazumdar and Ben Mahler.
>
>
> Bugs: MESOS-3182
> https://issues.apache.org/jira/browse/MESOS-3182
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Merged registerFramework() and reregisterFramework() (and their corresponding continuations) into subscribe() (and _subscribe()).
>
>
> Diffs
> -----
>
> src/master/master.hpp ea18c4e0bb0743747401b9cd5ea14ae9b56ae3cc
> src/master/master.cpp 351a3c2b5f551ad065682cea601d2436258e4544
>
> Diff: https://reviews.apache.org/r/37046/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>