mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ben Mahler" <benjamin.mah...@gmail.com>
Subject Re: Review Request 22162: Pulled common Framework (re-)registration validation into a helper method.
Date Thu, 05 Jun 2014 18:27:48 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message