Re: Review Request 70533: Implemented the UPDATE_FRAMEWORK call in the V1 API.

2019-05-23 Thread Andrei Sekretenko 


> On May 22, 2019, 7:52 p.m., Benjamin Mahler wrote:
> > src/master/master.cpp
> > Lines 3172-3173 (patched)
> > 
> >
> > This is undefined behavior (and may crash, this has caught me before), 
> > because `call` is moved in the same expression as the 
> > `call.framework_info()` is touched, and C++ doesn't not guarantee the order 
> > of evaluation.
> > 
> > We have to split it out:
> > 
> > ```
> >   Future authorized = authorizeFramework(call.framework_info());
> >   
> >   return authorized
> > .then(defer(self(), ::_updateFramework, std::move(call), 
> > lambda::_1));
> > ```

Yeah, and a crash could be a relatively good outcome of UB ;) Many thanks for 
catching this!


- Andrei


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70533/#review215466
---


On May 22, 2019, 1:49 p.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70533/
> ---
> 
> (Updated May 22, 2019, 1:49 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 1618794cdb9f44656e697de2abbf6c7e0b6fa50d 
>   src/master/master.hpp 5ad128d00d3cdf0dca89eb637ae196987bdce412 
>   src/master/master.cpp 7c9d9c3d566e29d3f8a5781ee6cdf11973a755e6 
>   src/master/validation.cpp 39f5bfdb39fd6944a3e893b1587e3c2e26818d27 
> 
> 
> Diff: https://reviews.apache.org/r/70533/diff/8/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 70533: Implemented the UPDATE_FRAMEWORK call in the V1 API.

2019-05-22 Thread Benjamin Mahler 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70533/#review215466
---


Fix it, then Ship it!




Nice and clean patch!

There's a bug below, but I'll get this fixed and committed shortly.


src/master/master.cpp
Lines 3172-3173 (patched)


This is undefined behavior (and may crash, this has caught me before), 
because `call` is moved in the same expression as the `call.framework_info()` 
is touched, and C++ doesn't not guarantee the order of evaluation.

We have to split it out:

```
  Future authorized = authorizeFramework(call.framework_info());
  
  return authorized
.then(defer(self(), ::_updateFramework, std::move(call), 
lambda::_1));
```


- Benjamin Mahler


On May 22, 2019, 1:49 p.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70533/
> ---
> 
> (Updated May 22, 2019, 1:49 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 1618794cdb9f44656e697de2abbf6c7e0b6fa50d 
>   src/master/master.hpp 5ad128d00d3cdf0dca89eb637ae196987bdce412 
>   src/master/master.cpp 7c9d9c3d566e29d3f8a5781ee6cdf11973a755e6 
>   src/master/validation.cpp 39f5bfdb39fd6944a3e893b1587e3c2e26818d27 
> 
> 
> Diff: https://reviews.apache.org/r/70533/diff/8/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 70533: Implemented the UPDATE_FRAMEWORK call in the V1 API.

2019-05-22 Thread Andrei Sekretenko 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70533/
---

(Updated May 22, 2019, 1:49 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Fixed indentation.


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 (updated)
-

  src/master/http.cpp 1618794cdb9f44656e697de2abbf6c7e0b6fa50d 
  src/master/master.hpp 5ad128d00d3cdf0dca89eb637ae196987bdce412 
  src/master/master.cpp 7c9d9c3d566e29d3f8a5781ee6cdf11973a755e6 
  src/master/validation.cpp 39f5bfdb39fd6944a3e893b1587e3c2e26818d27 


Diff: https://reviews.apache.org/r/70533/diff/8/

Changes: https://reviews.apache.org/r/70533/diff/7-8/


Testing
---


Thanks,

Andrei Sekretenko

Re: Review Request 70533: Implemented the UPDATE_FRAMEWORK call in the V1 API.

2019-05-21 Thread Andrei Sekretenko 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70533/
---

(Updated May 21, 2019, 1:54 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 (updated)
-

  src/master/http.cpp 1618794cdb9f44656e697de2abbf6c7e0b6fa50d 
  src/master/master.hpp bf7a6b4720ac2e7a072f145f6a27e96a1a9da41e 
  src/master/master.cpp 24bd9572741c033c02d03a3a3a35d7d4ed8b60de 
  src/master/validation.cpp 39f5bfdb39fd6944a3e893b1587e3c2e26818d27 


Diff: https://reviews.apache.org/r/70533/diff/7/

Changes: https://reviews.apache.org/r/70533/diff/6-7/


Testing
---


Thanks,

Andrei Sekretenko

Re: Review Request 70533: Implemented the UPDATE_FRAMEWORK call in the V1 API.

2019-05-17 Thread Andrei Sekretenko 


> On April 24, 2019, 7:52 p.m., Benjamin Mahler wrote:
> > src/master/master.cpp
> > Lines 3237 (patched)
> > 
> >
> > What does updateFramework do if the changes are not allowed? Crash?
> > 
> > Seems like we should be CHECKing that it succeeded or something..

Now it simply crashes - and validity of the framework_info is ensured by the 
code executed before it.


- Andrei


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70533/#review214864
---


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

Re: Review Request 70533: Implemented the UPDATE_FRAMEWORK call in the V1 API.

2019-05-17 Thread Andrei Sekretenko 


> On May 12, 2019, 5:46 a.m., Benjamin Mahler wrote:
> > src/master/master.cpp
> > Lines 2877-2880 (original), 2884-2887 (patched)
> > 
> >
> > 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)
> > 
> >
> > 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)
> > 
> >
> > 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
> 
>

Re: Review Request 70533: Implemented the UPDATE_FRAMEWORK call in the V1 API.

2019-05-17 Thread Andrei Sekretenko 

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


Summary (updated)
-

Implemented the UPDATE_FRAMEWORK call in the V1 API.


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 (updated)
-

  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/

Changes: https://reviews.apache.org/r/70533/diff/5-6/


Testing
---


Thanks,

Andrei Sekretenko