> On May 12, 2019, 5:46 a.m., Benjamin Mahler wrote: > > src/master/master.cpp > > Lines 2877-2880 (original), 2884-2887 (patched) > > <https://reviews.apache.org/r/70533/diff/5/?file=2142312#file2142312line2884> > > > > Hm.. why doesn't the broadcast function send it to subscribers too? why > > the need to have this separately done outside the function? > > > > Probably want to rename it if we're able to do both in the function: > > `broadcastFramwerkUpdate(...)`
There was one case in which only FRAMEWORK_UPDATED was sent - most likely, a bug. Moved this refactoring into a preceding patch: https://reviews.apache.org/r/70665/ > On May 12, 2019, 5:46 a.m., Benjamin Mahler wrote: > > src/master/master.cpp > > Lines 3279-3280 (patched) > > <https://reviews.apache.org/r/70533/diff/5/?file=2142312#file2142312line3279> > > > > This.. seems odd. Do we need to rename this function now that it's more > > a master-stateful validation of the framework info? > > > > Reading this code, I'm left thinking that the validation of the old vs > > new FrameworkInfo is accidentally omitted, but I'm guessing your previous > > patches actually put that within the `validateFrameworkSubscription(...)` > > function and that's how it's getting validated here? > > > > Seems like we need to do some renaming here, it seems to be more a > > stateful validation of the framework info now, not subscription. I've renaming the method into `Master::validateFramework()` (and would appreciate a better naming suggestion). Also, after looking at all this (including the race between two SUBSCRIBE calls), I decided to keep the validation against the existing FrameworkInfo separate from the all the other validation. See preceding patches https://reviews.apache.org/r/70666 and https://reviews.apache.org/r/70668 > On May 12, 2019, 5:46 a.m., Benjamin Mahler wrote: > > src/master/master.cpp > > Lines 3287-3291 (patched) > > <https://reviews.apache.org/r/70533/diff/5/?file=2142312#file2142312line3287> > > > > Seems unfortunate that this is getting caught here instead of within > > validation of the Call. It would be nice if this function can just CHECK it. Moved this into call validation. Now this method does not need the `frameworkId` at all. - Andrei ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70533/#review215205 ----------------------------------------------------------- On May 17, 2019, 3:18 p.m., Andrei Sekretenko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70533/ > ----------------------------------------------------------- > > (Updated May 17, 2019, 3:18 p.m.) > > > Review request for mesos and Benjamin Mahler. > > > Bugs: MESOS-7258 > https://issues.apache.org/jira/browse/MESOS-7258 > > > Repository: mesos > > > Description > ------- > > This patch is based on the previous implementation attempt: > https://reviews.apache.org/r/66229/ > > > Diffs > ----- > > src/master/http.cpp c2c7b9b65bd66679420f62d37b01d90e3692c71d > src/master/master.hpp c523c937e294eaffe0e58306c267770c119c9f42 > src/master/master.cpp c72b92656dddca63ee89abf676da66bd76f58a6d > src/master/validation.cpp 9fb0850987ce385d345302cac9721adead7181b8 > > > Diff: https://reviews.apache.org/r/70533/diff/6/ > > > Testing > ------- > > > Thanks, > > Andrei Sekretenko > >
