> On Dec. 23, 2014, 4:56 p.m., Niklas Nielsen wrote: > > src/watcher/whitelist_watcher.cpp, lines 60-61 > > <https://reviews.apache.org/r/28514/diff/2/?file=780056#file780056line60> > > > > Can you explain _why_ you only need to call the subscriber with an > > empty whitelist when an initial list was provided (not only the fact that > > it's happening). Does that mean that if the user doesn't specify a list, > > the subscriber won't be notified? I have a hard time understanding how this > > fixed the gmock issue and remains correct. > > Alexander Rukletsov wrote: > The expected logic (codified in tests and leading to gmock issues) is to > notify the subscriber once and only if a change to the previous state occurs. > If we do not want to use whitelist ("*" is passed) and the initial whitelist > was not provided (`intialWhitelist == None()`), the subscriber should not be > notified, because no change to the previous state occurs. However, if the > initial whitelist is, for example, set to empty (`intialWhitelist == {}`) for > security reasons, then passing "*" should trigger notification. > > Regarding you question, if the user does not specify a list > (`intialWhitelist` defaults to `None()`), the subscriber won't be notified > only if "*" is passed. > > Does it makes sense? > > Niklas Nielsen wrote: > > if the user does not specify a list (intialWhitelist defaults to > None()), the subscriber won't be notified only if "*" is passed. > > Initial whitelist is (when this patch lands) always None() and that is > maybe the thing that I have a hard time understanding. You have pointed me to > a dependent review which starts using it. Can you mark it as dependent on > this one? > The comment is confusing to me as the code below doesn't use the initial > list but is generalized to 'lastWhitelist' (which makes more sense to me). > The explanation I have been looking for is why the initial case needs to > be dealt with in a special way. > When would you set a initial whitelist which has not been read from a > file by the whitelist watcher? > > Also, how can 'lastWhitelist' ever be Some() when path is '*'? Does the > patch effectively just remove the 'subscriber(None());' line? > > Thanks for helping me understand this.
I'll update the comment for clarity. - Alexander ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28514/#review65954 ----------------------------------------------------------- On Dec. 2, 2014, 11:10 a.m., Alexander Rukletsov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/28514/ > ----------------------------------------------------------- > > (Updated Dec. 2, 2014, 11:10 a.m.) > > > Review request for mesos, Cody Maloney, Niklas Nielsen, and Vinod Kone. > > > Repository: mesos-git > > > Description > ------- > > The subscriber may now provide an initial whitelist and will be notified only > when the parsed whitelist differs from the initial one. The subscriber is not > explicitly notified that there is no whitelist unless they have provided a > valid initial whitelist before. This change suppresses gmock warnings for > uninteresting mock function calls. > > > Diffs > ----- > > src/watcher/whitelist_watcher.hpp 5838854 > src/watcher/whitelist_watcher.cpp 32713bb > > Diff: https://reviews.apache.org/r/28514/diff/ > > > Testing > ------- > > make check (Mac OS 10.9.4, Ubuntu 14.04) > checked test log for gmock warnings. > > > Thanks, > > Alexander Rukletsov > >
