You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Jiang Yan Xu <ya...@jxu.me> on 2014/06/03 06:33:04 UTC
Review Request 22162: Pulled common Framework (re-)registration validation
into a helper method.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22162/
-----------------------------------------------------------
Review request for mesos, Dominic Hamon and Vinod Kone.
Bugs: MESOS-1373
https://issues.apache.org/jira/browse/MESOS-1373
Repository: mesos-git
Description
-------
This is a follow up for https://reviews.apache.org/r/21787.
There exists duplication of logic in (re)-registerFramework methods.
Diffs
-----
src/master/master.hpp d4ef4bec7168179f2168e88d3727e50b0e2e68a1
src/master/master.cpp 766a0e36a6e7a615e7b2974d9fee70bcef446719
Diff: https://reviews.apache.org/r/22162/diff/
Testing
-------
make check.
Thanks,
Jiang Yan Xu
Re: Review Request 22162: Pulled common Framework (re-)registration
validation into a helper method.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22162/#review44600
-----------------------------------------------------------
src/master/master.cpp
<https://reviews.apache.org/r/22162/#comment79021>
Why an Option<Error> instead of a Try?
src/master/master.cpp
<https://reviews.apache.org/r/22162/#comment79022>
The "Framework is not authenticated" error is not "due to invalid FrameworkInfo".
- Adam B
On June 2, 2014, 9:33 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22162/
> -----------------------------------------------------------
>
> (Updated June 2, 2014, 9:33 p.m.)
>
>
> Review request for mesos, Dominic Hamon and Vinod Kone.
>
>
> Bugs: MESOS-1373
> https://issues.apache.org/jira/browse/MESOS-1373
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This is a follow up for https://reviews.apache.org/r/21787.
> There exists duplication of logic in (re)-registerFramework methods.
>
>
> Diffs
> -----
>
> src/master/master.hpp d4ef4bec7168179f2168e88d3727e50b0e2e68a1
> src/master/master.cpp 766a0e36a6e7a615e7b2974d9fee70bcef446719
>
> Diff: https://reviews.apache.org/r/22162/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 22162: Pulled common Framework (re-)registration
validation into a helper method.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22162/#review44631
-----------------------------------------------------------
Ship it!
Ship It!
- Dominic Hamon
On June 3, 2014, 8:30 a.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22162/
> -----------------------------------------------------------
>
> (Updated June 3, 2014, 8:30 a.m.)
>
>
> Review request for mesos, Adam B, Dominic Hamon, and Vinod Kone.
>
>
> Bugs: MESOS-1373
> https://issues.apache.org/jira/browse/MESOS-1373
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This is a follow up for https://reviews.apache.org/r/21787.
> There exists duplication of logic in (re)-registerFramework methods.
>
>
> Diffs
> -----
>
> src/master/master.hpp d4ef4bec7168179f2168e88d3727e50b0e2e68a1
> src/master/master.cpp 766a0e36a6e7a615e7b2974d9fee70bcef446719
>
> Diff: https://reviews.apache.org/r/22162/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 22162: Pulled common Framework (re-)registration
validation into a helper method.
Posted by Ben Mahler <be...@gmail.com>.
> On June 4, 2014, 10:50 p.m., Ben Mahler wrote:
> > src/master/master.hpp, lines 622-630
> > <https://reviews.apache.org/r/22162/diff/2/?file=602867#file602867line622>
> >
> > What is the longer term strategy for message validation? Having a single generically named 'validate' suggests a large number of overloads or other handlers? Isn't 'validate' a bit too generic for only validating framework registration and re-registration?
> >
> > Probably we should think carefully about how we want to do message validation in a more general sense in the master.
>
> Jiang Yan Xu wrote:
> My thinking is: the verb validate is general but its parameter list distinguishes it. Other overloads can be named "validate" too.
> So in the future if there are more *common* things that should be validated for all Messages that contain FrameworkInfo, they can be put in this method too. That is not to say that there shouldn't be message handler specific checking within these handlers themselves.
>
> So I personally don't think "validate" is too general and our code base usually chooses succinctness over descriptiveness (which I don't think works best in all cases) but anyhow I am open to suggestion for a better name.
>
> WRT tolerating the redundancy vs. a private method used by not many callers, I was reluctant at first but these identical code chunks do appear inelegant...
>
> Thoughts?
As an aside, wouldn't an Option<Error> better express the result of a validation? Semantically they are the same, but in general we use 'Try' to represent an operation that can fail. With validation, it's subtle but it seems that the validation operation itself doesn't fail, rather it's returning a validation result to the caller. The result is an optional validation error, right? This is the pattern used in a few places, including the task/offer visitors (Option<string> was used because 'Error' didn't yet exist :)).
With respect to naming, it seems unfortunate that without the comment above the method, I would have a hard time interpreting its purpose. For example, if the signature was 'validate(Message m)' I could infer purpose easily.
I'm more interested in understanding this validation pattern and how it's going to scale out to all the messages. I haven't thought about it very hard, but there are ways of doing this that seem cleaner IMO. For example, you could imagine that 'install' takes an optional 'MessageValidator'. It might be nice to be able to go a more declarative route for message validation.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22162/#review44770
-----------------------------------------------------------
On June 3, 2014, 3:30 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22162/
> -----------------------------------------------------------
>
> (Updated June 3, 2014, 3:30 p.m.)
>
>
> Review request for mesos, Adam B, Dominic Hamon, and Vinod Kone.
>
>
> Bugs: MESOS-1373
> https://issues.apache.org/jira/browse/MESOS-1373
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This is a follow up for https://reviews.apache.org/r/21787.
> There exists duplication of logic in (re)-registerFramework methods.
>
>
> Diffs
> -----
>
> src/master/master.hpp d4ef4bec7168179f2168e88d3727e50b0e2e68a1
> src/master/master.cpp 766a0e36a6e7a615e7b2974d9fee70bcef446719
>
> Diff: https://reviews.apache.org/r/22162/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 22162: Pulled common Framework (re-)registration
validation into a helper method.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On June 4, 2014, 3:50 p.m., Ben Mahler wrote:
> > src/master/master.hpp, lines 622-630
> > <https://reviews.apache.org/r/22162/diff/2/?file=602867#file602867line622>
> >
> > What is the longer term strategy for message validation? Having a single generically named 'validate' suggests a large number of overloads or other handlers? Isn't 'validate' a bit too generic for only validating framework registration and re-registration?
> >
> > Probably we should think carefully about how we want to do message validation in a more general sense in the master.
My thinking is: the verb validate is general but its parameter list distinguishes it. Other overloads can be named "validate" too.
So in the future if there are more *common* things that should be validated for all Messages that contain FrameworkInfo, they can be put in this method too. That is not to say that there shouldn't be message handler specific checking within these handlers themselves.
So I personally don't think "validate" is too general and our code base usually chooses succinctness over descriptiveness (which I don't think works best in all cases) but anyhow I am open to suggestion for a better name.
WRT tolerating the redundancy vs. a private method used by not many callers, I was reluctant at first but these identical code chunks do appear inelegant...
Thoughts?
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22162/#review44770
-----------------------------------------------------------
On June 3, 2014, 8:30 a.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22162/
> -----------------------------------------------------------
>
> (Updated June 3, 2014, 8:30 a.m.)
>
>
> Review request for mesos, Adam B, Dominic Hamon, and Vinod Kone.
>
>
> Bugs: MESOS-1373
> https://issues.apache.org/jira/browse/MESOS-1373
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This is a follow up for https://reviews.apache.org/r/21787.
> There exists duplication of logic in (re)-registerFramework methods.
>
>
> Diffs
> -----
>
> src/master/master.hpp d4ef4bec7168179f2168e88d3727e50b0e2e68a1
> src/master/master.cpp 766a0e36a6e7a615e7b2974d9fee70bcef446719
>
> Diff: https://reviews.apache.org/r/22162/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 22162: Pulled common Framework (re-)registration
validation into a helper method.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22162/#review44770
-----------------------------------------------------------
src/master/master.hpp
<https://reviews.apache.org/r/22162/#comment79313>
What is the longer term strategy for message validation? Having a single generically named 'validate' suggests a large number of overloads or other handlers? Isn't 'validate' a bit too generic for only validating framework registration and re-registration?
Probably we should think carefully about how we want to do message validation in a more general sense in the master.
- Ben Mahler
On June 3, 2014, 3:30 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22162/
> -----------------------------------------------------------
>
> (Updated June 3, 2014, 3:30 p.m.)
>
>
> Review request for mesos, Adam B, Dominic Hamon, and Vinod Kone.
>
>
> Bugs: MESOS-1373
> https://issues.apache.org/jira/browse/MESOS-1373
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This is a follow up for https://reviews.apache.org/r/21787.
> There exists duplication of logic in (re)-registerFramework methods.
>
>
> Diffs
> -----
>
> src/master/master.hpp d4ef4bec7168179f2168e88d3727e50b0e2e68a1
> src/master/master.cpp 766a0e36a6e7a615e7b2974d9fee70bcef446719
>
> Diff: https://reviews.apache.org/r/22162/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 22162: Pulled common Framework (re-)registration
validation into a helper method.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22162/#review44637
-----------------------------------------------------------
Ship it!
Ship It!
- Adam B
On June 3, 2014, 8:30 a.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22162/
> -----------------------------------------------------------
>
> (Updated June 3, 2014, 8:30 a.m.)
>
>
> Review request for mesos, Adam B, Dominic Hamon, and Vinod Kone.
>
>
> Bugs: MESOS-1373
> https://issues.apache.org/jira/browse/MESOS-1373
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This is a follow up for https://reviews.apache.org/r/21787.
> There exists duplication of logic in (re)-registerFramework methods.
>
>
> Diffs
> -----
>
> src/master/master.hpp d4ef4bec7168179f2168e88d3727e50b0e2e68a1
> src/master/master.cpp 766a0e36a6e7a615e7b2974d9fee70bcef446719
>
> Diff: https://reviews.apache.org/r/22162/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 22162: Pulled common Framework (re-)registration
validation into a helper method.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22162/#review44725
-----------------------------------------------------------
Ship it!
Ship It!
- Vinod Kone
On June 3, 2014, 3:30 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22162/
> -----------------------------------------------------------
>
> (Updated June 3, 2014, 3:30 p.m.)
>
>
> Review request for mesos, Adam B, Dominic Hamon, and Vinod Kone.
>
>
> Bugs: MESOS-1373
> https://issues.apache.org/jira/browse/MESOS-1373
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This is a follow up for https://reviews.apache.org/r/21787.
> There exists duplication of logic in (re)-registerFramework methods.
>
>
> Diffs
> -----
>
> src/master/master.hpp d4ef4bec7168179f2168e88d3727e50b0e2e68a1
> src/master/master.cpp 766a0e36a6e7a615e7b2974d9fee70bcef446719
>
> Diff: https://reviews.apache.org/r/22162/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 22162: Pulled common Framework (re-)registration
validation into a helper method.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22162/
-----------------------------------------------------------
(Updated June 3, 2014, 8:30 a.m.)
Review request for mesos, Adam B, Dominic Hamon, and Vinod Kone.
Changes
-------
Addressed comments.
Bugs: MESOS-1373
https://issues.apache.org/jira/browse/MESOS-1373
Repository: mesos-git
Description
-------
This is a follow up for https://reviews.apache.org/r/21787.
There exists duplication of logic in (re)-registerFramework methods.
Diffs (updated)
-----
src/master/master.hpp d4ef4bec7168179f2168e88d3727e50b0e2e68a1
src/master/master.cpp 766a0e36a6e7a615e7b2974d9fee70bcef446719
Diff: https://reviews.apache.org/r/22162/diff/
Testing
-------
make check.
Thanks,
Jiang Yan Xu