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

Re: Review Request 70533: Implemented the UPDATE_FRAMEWORK call.

2019-05-11 Thread Benjamin Mahler 

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




src/master/master.hpp
Lines 665-668 (patched)


No need to re-iterate all these details here, we're unlikely to keep it in 
sync with what the code does.



src/master/master.hpp
Lines 674 (patched)


s/make/Make/



src/master/master.hpp
Lines 676 (patched)


s/is/it/



src/master/master.hpp
Lines 677-678 (patched)


The early newline makes it look like this last sentence is potentially 
separate from the TODO when reading this, how about just continuing from the 
previous line?

```
  // currently it is not, see MESOS-9746. At that point, we can
  // potentially remove the `broadcastFrameworkUpdate()` function.
```



src/master/master.hpp
Lines 685 (patched)


The arguments are generally named in these declarations, we should stay 
consistent with that



src/master/master.hpp
Lines 1073-1079 (patched)


The arguments are generally named in the other declarations, we should stay 
consistent with that



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(...)`



src/master/master.cpp
Lines 2886-2894 (original), 2901-2909 (patched)


Seems like we can construct the message outside the loop? Feel free to 
follow up with a patch later for that or have a patch in front of this patch.



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.



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.



src/master/master.cpp
Lines 3309 (patched)


Start all TODOs with a capital letter, you should find that's the norm if 
you grep the codebase



src/master/master.cpp
Lines 3327 (patched)


Can we be more clear here?

Are we referring to concurrent SUBSCRIBE or UPDATE_FRAMEWORK calls?

It seems to me we're relying on authorization preserving the ordering



src/master/master.cpp
Lines 3338 (patched)


no double newline here



src/master/master.cpp
Lines 3345 (patched)


newline


- Benjamin Mahler


On May 7, 2019, 1:09 p.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70533/
> ---
> 
> (Updated May 7, 2019, 1:09 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/java/jni/org_apache_mesos_v1_scheduler_V0Mesos.cpp 
> 35a04a3d0183697f93dadb3cbfead3ee0c2fea08 
>   src/master/http.cpp 765bbf1442f26dcc08c5404f714b6a2ef6616ed8 
>   src/master/master.hpp 7d9732f1e432f6f0290d234242864cbdbf381fa8 
>   src/master/master.cpp a8ee6297e1587c160a47b806914d3c3aa4f35cd7 
>   src/master/validation.cpp 9fb0850987ce385d345302cac9721adead7181b8 
> 
> 
> Diff: https://reviews.apache.org/r/70533/diff/5/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 70533: Implemented the UPDATE_FRAMEWORK call.

2019-05-07 Thread Andrei Sekretenko 

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

(Updated May 7, 2019, 1:09 p.m.)


Review request for mesos and Benjamin Mahler.


Bugs: MESOS-7258
https://issues.apache.org/jira/browse/MESOS-7258


Repository: mesos


Description (updated)
---

This patch is based on the previous implementation attempt:
https://reviews.apache.org/r/66229/


Diffs
-

  src/java/jni/org_apache_mesos_v1_scheduler_V0Mesos.cpp 
35a04a3d0183697f93dadb3cbfead3ee0c2fea08 
  src/master/http.cpp 765bbf1442f26dcc08c5404f714b6a2ef6616ed8 
  src/master/master.hpp 7d9732f1e432f6f0290d234242864cbdbf381fa8 
  src/master/master.cpp a8ee6297e1587c160a47b806914d3c3aa4f35cd7 
  src/master/validation.cpp 9fb0850987ce385d345302cac9721adead7181b8 


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


Testing
---


Thanks,

Andrei Sekretenko

Re: Review Request 70533: Implemented the UPDATE_FRAMEWORK call.

2019-05-02 Thread Andrei Sekretenko 

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

(Updated May 2, 2019, 4:03 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Added sending updates to slaves and master API subscribers.

Added re-validation of the call after authorization to avoid possible races 
with concurrent changes of the master's state. (Unfortunately, I cannot simply 
remove the first validation, because the authorizer needs a valid 
FrameworkInfo).


Bugs: MESOS-7258
https://issues.apache.org/jira/browse/MESOS-7258


Repository: mesos


Description
---

Implemented the UPDATE_FRAMEWORK call.


Diffs (updated)
-

  src/java/jni/org_apache_mesos_v1_scheduler_V0Mesos.cpp 
35a04a3d0183697f93dadb3cbfead3ee0c2fea08 
  src/master/http.cpp 765bbf1442f26dcc08c5404f714b6a2ef6616ed8 
  src/master/master.hpp 7d9732f1e432f6f0290d234242864cbdbf381fa8 
  src/master/master.cpp a8ee6297e1587c160a47b806914d3c3aa4f35cd7 
  src/master/validation.cpp 9fb0850987ce385d345302cac9721adead7181b8 


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

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


Testing
---


Thanks,

Andrei Sekretenko

Re: Review Request 70533: Implemented the UPDATE_FRAMEWORK call.

2019-05-02 Thread Andrei Sekretenko 

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

(Updated May 2, 2019, 3:58 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Added sending updates to slaves and master API subscribers.

Added re-validation of the call after authorization to avoid possible races 
with concurrent changes of the master's state. (Unfortunately, I cannot simply 
remove the first validation, because the authorizer needs a valid 
FrameworkInfo).


Bugs: MESOS-7258
https://issues.apache.org/jira/browse/MESOS-7258


Repository: mesos


Description
---

Implemented the UPDATE_FRAMEWORK call.


Diffs (updated)
-

  src/java/jni/org_apache_mesos_v1_scheduler_V0Mesos.cpp 
35a04a3d0183697f93dadb3cbfead3ee0c2fea08 
  src/master/http.cpp 765bbf1442f26dcc08c5404f714b6a2ef6616ed8 
  src/master/master.hpp 7d9732f1e432f6f0290d234242864cbdbf381fa8 
  src/master/master.cpp a8ee6297e1587c160a47b806914d3c3aa4f35cd7 
  src/master/validation.cpp 9fb0850987ce385d345302cac9721adead7181b8 


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

Changes: https://reviews.apache.org/r/70533/diff/3-4/


Testing
---


Thanks,

Andrei Sekretenko

Re: Review Request 70533: Implemented the UPDATE_FRAMEWORK call.

2019-04-25 Thread Andrei Sekretenko 

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

(Updated April 25, 2019, 8:14 p.m.)


Review request for mesos and Benjamin Mahler.


Bugs: MESOS-7258
https://issues.apache.org/jira/browse/MESOS-7258


Repository: mesos


Description
---

Implemented the UPDATE_FRAMEWORK call.


Diffs (updated)
-

  src/java/jni/org_apache_mesos_v1_scheduler_V0Mesos.cpp 
35a04a3d0183697f93dadb3cbfead3ee0c2fea08 
  src/master/http.cpp 765bbf1442f26dcc08c5404f714b6a2ef6616ed8 
  src/master/master.hpp 7d9732f1e432f6f0290d234242864cbdbf381fa8 
  src/master/master.cpp 7dcdc9ab62a46638a027eb9a54c1dff173785927 
  src/master/validation.cpp d7f210fc1ed228113c7f97bce9a43916840b2252 


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

Changes: https://reviews.apache.org/r/70533/diff/2-3/


Testing
---


Thanks,

Andrei Sekretenko

Re: Review Request 70533: Implemented the UPDATE_FRAMEWORK call.

2019-04-25 Thread Andrei Sekretenko 

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

(Updated April 25, 2019, 8:04 p.m.)


Review request for mesos and Benjamin Mahler.


Bugs: MESOS-7258
https://issues.apache.org/jira/browse/MESOS-7258


Repository: mesos


Description (updated)
---

Implemented the UPDATE_FRAMEWORK call.


Diffs (updated)
-

  src/java/jni/org_apache_mesos_v1_scheduler_V0Mesos.cpp 
35a04a3d0183697f93dadb3cbfead3ee0c2fea08 
  src/master/http.cpp 765bbf1442f26dcc08c5404f714b6a2ef6616ed8 
  src/master/master.hpp 7d9732f1e432f6f0290d234242864cbdbf381fa8 
  src/master/master.cpp 7dcdc9ab62a46638a027eb9a54c1dff173785927 
  src/master/validation.cpp d7f210fc1ed228113c7f97bce9a43916840b2252 


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

Changes: https://reviews.apache.org/r/70533/diff/1-2/


Testing
---


Thanks,

Andrei Sekretenko

Re: Review Request 70533: Implemented the UPDATE_FRAMEWORK call.

2019-04-24 Thread Benjamin Mahler 

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




src/java/jni/org_apache_mesos_v1_scheduler_V0Mesos.cpp
Lines 687-691 (patched)


Shouldn't we just include this in the same case block above as the other 
calls that aren't supported in v0? Why log it differently?



src/master/master.cpp
Lines 3206 (patched)


`std::move(call)` ?



src/master/master.cpp
Lines 3212 (patched)


`const Future&` ?



src/master/master.cpp
Lines 3215-3216 (patched)


should be 2 space indents within a new scope



src/master/master.cpp
Lines 3220-3223 (patched)


Hm.. perhaps a TODO here for us to be able to know which roles the 
framework was not authorized to subscribe to so that we can print them?



src/master/master.cpp
Lines 3229 (patched)


Right now some of the messages are including periods and some are not, it 
seems simpler not to include them



src/master/master.cpp
Lines 3232-3235 (patched)


no std:: prefix in the .cpp file



src/master/master.cpp
Lines 3232-3235 (patched)


Can you std::move them out from the protobuf into the std::set to avoid the 
copying?



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


- Benjamin Mahler


On April 23, 2019, 7:06 p.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70533/
> ---
> 
> (Updated April 23, 2019, 7:06 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7258
> https://issues.apache.org/jira/browse/MESOS-7258
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented UPDATE_FRAMEWORK scheduler call.
> 
> This call allows to perform the same changes in FrameworkInfo as 
> resubscribing a framework.
> 
> HTTP return codes specific to this call:
> 200 OK on success
> 400 Bad request when the requested update is not valid.
> 403 Unathorized when the framework would not be authorized to use some 
> entities (currently roles) after the requested update.
> 409 Conflict when the framework is removed by a concurrent call.
> 
> No "incomplete updates" occur when an update is invalid. I.e. the update 
> either succeeds or fails completely.
> 
> An attempt to change framework's user/checkpointing ability is not swallowed 
> silently, but is treated as an error - this is different from attempting to 
> change these properties by resubscribing a framework.
> 
> 
> Diffs
> -
> 
>   src/java/jni/org_apache_mesos_v1_scheduler_V0Mesos.cpp 
> 35a04a3d0183697f93dadb3cbfead3ee0c2fea08 
>   src/master/http.cpp e7a92d0f554ba4cafaee5a75f09b46eb1bf4a310 
>   src/master/master.hpp 94891af9deeaddbfc9d6eabb243aed97f7b7 
>   src/master/master.cpp ad54ae217863a08f4e6d743b39c176b171353084 
>   src/master/validation.cpp f032a781608857d0c9cfa220dd8d70f74d60f1ec 
> 
> 
> Diff: https://reviews.apache.org/r/70533/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 70533: Implemented the UPDATE_FRAMEWORK call.

2019-04-23 Thread Andrei Sekretenko 

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

(Updated April 23, 2019, 7:06 p.m.)


Review request for mesos and Benjamin Mahler.


Bugs: MESOS-7258
https://issues.apache.org/jira/browse/MESOS-7258


Repository: mesos


Description
---

Implemented UPDATE_FRAMEWORK scheduler call.

This call allows to perform the same changes in FrameworkInfo as resubscribing 
a framework.

HTTP return codes specific to this call:
200 OK on success
400 Bad request when the requested update is not valid.
403 Unathorized when the framework would not be authorized to use some entities 
(currently roles) after the requested update.
409 Conflict when the framework is removed by a concurrent call.

No "incomplete updates" occur when an update is invalid. I.e. the update either 
succeeds or fails completely.

An attempt to change framework's user/checkpointing ability is not swallowed 
silently, but is treated as an error - this is different from attempting to 
change these properties by resubscribing a framework.


Diffs
-

  src/java/jni/org_apache_mesos_v1_scheduler_V0Mesos.cpp 
35a04a3d0183697f93dadb3cbfead3ee0c2fea08 
  src/master/http.cpp e7a92d0f554ba4cafaee5a75f09b46eb1bf4a310 
  src/master/master.hpp 94891af9deeaddbfc9d6eabb243aed97f7b7 
  src/master/master.cpp ad54ae217863a08f4e6d743b39c176b171353084 
  src/master/validation.cpp f032a781608857d0c9cfa220dd8d70f74d60f1ec 


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


Testing
---


Thanks,

Andrei Sekretenko