mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrei Sekretenko <asekrete...@mesosphere.io>
Subject Re: Review Request 70533: Implemented the UPDATE_FRAMEWORK call in the V1 API.
Date Fri, 17 May 2019 16:15:01 GMT


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


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