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