> On Dec. 2, 2014, 1:02 p.m., Ben Mahler wrote: > > I think I agree with Vinod that this is just movement without adding any > > value, for now, given that these are not independent of the Master. > > > > In fact, to understand these, one needs to understand the internals of the > > Master, which now requires even more non-local reasoning (need to be > > jumping across files to understand these). IMO this makes it harder to > > understand, and I would punt on this change in favor of first trying to > > clean these up to make them unit-testable. Until then, the loss of local > > reasoning in this change seems really unfortunate. > > > > This is not yours, but the existing names could be improved. What is a > > "checker"? And yes, these are "Visitors", but they are all ultimately for > > validation. > > So, how about we call these Validators? TaskIDValidator is a TaskValidator, > > UniqueOfferIDValidator is an OfferValidator.
There's huge value in removing code from a source file and localising them in another. From a discoverability and readability point of view, at least. Smaller, localised, files allow for better reasoning, better discoverability, and less impactful changes. Ie, changing the logic in one of these checkers shouldn't be a change to master.cpp. We are adding more validators and every one makes master.cpp larger, heavier, and harder to navigate. Yes, these are used by the master internally, but that's why they're in the master folder in src and in the relevant namespace. The logical extension of your reasoning is that all code relating to the master should be in one file, which is obviously (i think) a mistake. I'm happy to rename the classes in a followup patch, but we could bikeshed that for a while so I don't want to conflate them. - Dominic ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20423/#review63577 ----------------------------------------------------------- On Dec. 2, 2014, 12:36 p.m., Dominic Hamon wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/20423/ > ----------------------------------------------------------- > > (Updated Dec. 2, 2014, 12:36 p.m.) > > > Review request for mesos and Benjamin Hindman. > > > Bugs: MESOS-1064 > https://issues.apache.org/jira/browse/MESOS-1064 > > > Repository: mesos-git > > > Description > ------- > > This is the first step toward being able to write independent unit tests for > the validation visitors. > > > Diffs > ----- > > src/Makefile.am 86161fe7a8bdd86958d24adb74d434cd92d7dfb8 > src/master/master.cpp 8fcda4b9b5857f14cff8f6af2de31cca0208b88d > src/master/offer_checker.hpp PRE-CREATION > src/master/offer_checker.cpp PRE-CREATION > src/master/task_checker.hpp PRE-CREATION > src/master/task_checker.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/20423/diff/ > > > Testing > ------- > > make check. > ran Java test framework. > > > Thanks, > > Dominic Hamon > >
