Re: Review Request 70534: Added tests for the V1 UPDATE_FRAMEWORK call.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [70532, 70663, 70664, 70665, 70666, 70667, 70668, 70669, 
70670, 70533, 70671, 70534]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose --disable-libtool-wrappers 
--disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker-build.sh

- Mesos Reviewbot


On May 17, 2019, 8:19 a.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70534/
> ---
> 
> (Updated May 17, 2019, 8:19 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7258
> https://issues.apache.org/jira/browse/MESOS-7258
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added tests for the V1 UPDATE_FRAMEWORK call.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 5f97523fbe2d80733fbdcc7706f2761f5a071f9f 
>   src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6 
>   src/tests/master/update_framework_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/70534/diff/5/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 70651: Notifies master `/api/v1` subscribers about recovered frameworks.

[Chun-Hung Hsiao]

> On May 17, 2019, 10:16 a.m., Greg Mann wrote:
> > src/tests/api_tests.cpp
> > Lines 2774 (patched)
> > 
> >
> > Nit: make this a const ref?

This is copied because `event` is overwritten later. Do you think that it's 
okay to use a ref, since we don't access to it afterwards?


- Chun-Hung


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


On May 16, 2019, 5:11 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70651/
> ---
> 
> (Updated May 16, 2019, 5:11 a.m.)
> 
> 
> Review request for mesos and Greg Mann.
> 
> 
> Bugs: MESOS-9785
> https://issues.apache.org/jira/browse/MESOS-9785
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If one subscribes to master's `/api/v1` endpoint after a master failover
> but before an agent reregistration, frameworks recovered through the
> agent registration should be notified to the subscriber, otherwise
> recovered tasks will have framework IDs referring to frameworks unknown
> to the subscriber.
> 
> 
> Diffs
> -
> 
>   src/common/protobuf_utils.cpp 8b252cb11e17356836988dfc44a63953579a1def 
>   src/master/master.cpp 6c0e30bff3fbfe0ad1f3af44eb3df6f82048f0e1 
>   src/tests/api_tests.cpp bc19d7e9661f091aebf3072967548c7c3196239d 
> 
> 
> Diff: https://reviews.apache.org/r/70651/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 70660: Fix the XFS build for recent Fedora versions.

[Xudong Ni via Review Board]

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


Ship it!




Ship It!

- Xudong Ni


On May 17, 2019, 2:24 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70660/
> ---
> 
> (Updated May 17, 2019, 2:24 a.m.)
> 
> 
> Review request for mesos, Xudong Ni, Gilbert Song, Jie Yu, and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> On Fedora 30, we need to include  to get the right
> types, since  is no longer self-contained. For
> earlier versions, it is still safe to include both headers if
> they are available, though h> may need to be
> obtained via the xfsprogs-qa-devel package.
> 
> 
> Diffs
> -
> 
>   configure.ac b4bad5716986e2f7c132c6515179a65ccbfdaeac 
>   src/slave/containerizer/mesos/isolators/xfs/utils.hpp 
> db1829abbaac6113d39e71673403afe75b5ee738 
> 
> 
> Diff: https://reviews.apache.org/r/70660/diff/1/
> 
> 
> Testing
> ---
> 
> make check (Fedota 30)
> make (CentOS 6)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 70530: Refactored Framework updates for the UPDATE_FRAMEWORK call; fixed race.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [70583, 70531, 70530]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose --disable-libtool-wrappers 
--disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker-build.sh

- Mesos Reviewbot


On May 10, 2019, 2:50 p.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70530/
> ---
> 
> (Updated May 10, 2019, 2:50 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7258
> https://issues.apache.org/jira/browse/MESOS-7258
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The main concern of this patch is the behaviour of the framework
> re-subscription and of the upcoming UPDATE_FRAMEWORK call in cases when
> the framework attempts to change the "immutable" fields of
> FrameworkInfo, namely: `user`, `checkpoint`, `principal` and `id`.
> 
> The changes introduced by this patch:
> - The method `Framework::update()` is simplified: the logic of ignoring
>   the immutable fields is removed, this method fails the program if the
>   new values differ from the old ones. This is needed to simplify
>   keeping UPDATE_FRAMEWORK consistent with re-subscription.
> - The method `Master::validateFrameworkSubscription()` now returns
>   validation errors on attempts to change `user` or `checkpoint` fields.
>   This is needed to enable validating them in the UPDATE_FRAMEWORK call.
> - A method `Master::sanitizeFrameworkSubscription()` is introduced to
>   preserve the legacy behaviour of re-subscription (which ignores the
>   updates of `user` and `checkpoint`).
> - A second call of `validateFrameworkSubscription()` is performed after
>   the authorization. This is a crude fix of the already existing race
>   between two re-subscriptions against an empty master (see MESOS-9763).
>   It is necessary because otherwise the change in `Framework::update()`
>   would exacerbate this race (namely, it would be possible to crash the
>   master).
> 
> 
> Diffs
> -
> 
>   src/master/framework.cpp 05f5514c589b2dba08afe77281e5fbc4e29f232b 
>   src/master/master.hpp 7d9732f1e432f6f0290d234242864cbdbf381fa8 
>   src/master/master.cpp a8ee6297e1587c160a47b806914d3c3aa4f35cd7 
>   src/master/validation.hpp 95638a17052ece6c957aa76e4cead8d7bfe82024 
>   src/master/validation.cpp 9fb0850987ce385d345302cac9721adead7181b8 
> 
> 
> Diff: https://reviews.apache.org/r/70530/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 70660: Fix the XFS build for recent Fedora versions.

[Jiang Yan Xu]

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


Ship it!




Ship It!

- Jiang Yan Xu


On May 16, 2019, 7:24 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70660/
> ---
> 
> (Updated May 16, 2019, 7:24 p.m.)
> 
> 
> Review request for mesos, Xudong Ni, Gilbert Song, Jie Yu, and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> On Fedora 30, we need to include  to get the right
> types, since  is no longer self-contained. For
> earlier versions, it is still safe to include both headers if
> they are available, though h> may need to be
> obtained via the xfsprogs-qa-devel package.
> 
> 
> Diffs
> -
> 
>   configure.ac b4bad5716986e2f7c132c6515179a65ccbfdaeac 
>   src/slave/containerizer/mesos/isolators/xfs/utils.hpp 
> db1829abbaac6113d39e71673403afe75b5ee738 
> 
> 
> Diff: https://reviews.apache.org/r/70660/diff/1/
> 
> 
> Testing
> ---
> 
> make check (Fedota 30)
> make (CentOS 6)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 70534: Added tests for the V1 UPDATE_FRAMEWORK call.

[Andrei Sekretenko]

> On May 12, 2019, 5:44 a.m., Benjamin Mahler wrote:
> > src/tests/update_framework_tests.cpp
> > Lines 98 (patched)
> > 
> >
> > What does the presence of agent reservations matter to testing 
> > UPDATE_FRAMEWORK?

Indeed, reservations are not necessary for testing that allocator gets the 
update.

We can update from no roles to one role and check for an offer - I rewrote the 
test.


> On May 12, 2019, 5:44 a.m., Benjamin Mahler wrote:
> > src/tests/update_framework_tests.cpp
> > Lines 390-421 (patched)
> > 
> >
> > Is there no mock for this yet? How was the event streaming API tested 
> > without it?
> > 
> > How about pulling this out in front of this patch into its own header 
> > so that we can review it on its own? It's independent and adds to the 
> > complexity of this review.

It was tested by explicitly reading out every event (example: 
https://github.com/apache/mesos/blob/91fa416bf0d6191bb1a5e1fa3c883e23c98e892c/src/tests/api_tests.cpp#L3396).
Surely, each time a new event type was introduced, one had to modify unrelated 
tests: https://reviews.apache.org/r/61262/diff/8#index_header

Moved this mock into https://reviews.apache.org/r/70671/


- Andrei


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


On May 17, 2019, 3:19 p.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70534/
> ---
> 
> (Updated May 17, 2019, 3:19 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7258
> https://issues.apache.org/jira/browse/MESOS-7258
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added tests for the V1 UPDATE_FRAMEWORK call.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 5f97523fbe2d80733fbdcc7706f2761f5a071f9f 
>   src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6 
>   src/tests/master/update_framework_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/70534/diff/5/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

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

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

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

Review Request 70671: Added a class for setting expectations on master V1 API events in tests.

[Andrei Sekretenko]

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

Review request for mesos and Benjamin Mahler.


Repository: mesos


Description
---

This patch introduces a class with mock methods for subscribing to the
events of master's V1 streaming API and setting expectations for them.


Diffs
-

  src/Makefile.am 5f97523fbe2d80733fbdcc7706f2761f5a071f9f 
  src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6 
  src/tests/mock_master_api_subscriber.hpp PRE-CREATION 
  src/tests/mock_master_api_subscriber.cpp PRE-CREATION 


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


Testing
---


Thanks,

Andrei Sekretenko

Re: Review Request 70534: Added tests for the V1 UPDATE_FRAMEWORK call.

[Andrei Sekretenko]

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

(Updated May 17, 2019, 3:19 p.m.)


Review request for mesos and Benjamin Mahler.


Summary (updated)
-

Added tests for the V1 UPDATE_FRAMEWORK call.


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


Repository: mesos


Description (updated)
---

Added tests for the V1 UPDATE_FRAMEWORK call.


Diffs (updated)
-

  src/Makefile.am 5f97523fbe2d80733fbdcc7706f2761f5a071f9f 
  src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6 
  src/tests/master/update_framework_tests.cpp PRE-CREATION 


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

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


Testing
---


Thanks,

Andrei Sekretenko

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

[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

Review Request 70670: Simplified the `Framework::update()` method.

[Andrei Sekretenko]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

Now that ignoring some fields on re-subscription is performed outside
of this method, we can replace the fragile filed-by-field copying
of `FrameworkInfo` on updates with copying it as a whole.


Diffs
-

  src/master/framework.cpp 05f5514c589b2dba08afe77281e5fbc4e29f232b 


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


Testing
---


Thanks,

Andrei Sekretenko

Review Request 70669: Made it possible to validate against `user` and `checkpoint` updates.

[Andrei Sekretenko]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

This patch adds `user` and `checkpoint` fields of `FrameworkInfo`
into the `validateUpdate()` function for the purpose of using this
function to implement the UPDATE_FRAMEWORK call. To preserve the legacy
behaviour of re-subscription which silently ignores these fields,
the re-subscription path now explicitly sets them to their old values.


Diffs
-

  src/master/master.cpp c72b92656dddca63ee89abf676da66bd76f58a6d 
  src/master/validation.hpp 95638a17052ece6c957aa76e4cead8d7bfe82024 
  src/master/validation.cpp 9fb0850987ce385d345302cac9721adead7181b8 
  src/tests/master_validation_tests.cpp 
1b7a8273f2704e0a4dedf33c55ede33dc1b1a4af 


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


Testing
---


Thanks,

Andrei Sekretenko

Review Request 70668: Fixed the race between validating and applying FrameworkInfo updates.

[Andrei Sekretenko]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

This patch moves the FrameworkInfo update validation into the
continuation of the SUBSCRIBE calls (after authorization). This fixes
the race between two concurrent SUBSCRIBE calls attempting to change
principal (see MESOS-9763) and prevents the future changes of the update
validation from exacerbating this race.


Diffs
-

  src/master/master.cpp c72b92656dddca63ee89abf676da66bd76f58a6d 


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


Testing
---


Thanks,

Andrei Sekretenko

Review Request 70667: Added unit tests for 'framework::validateUpdate()'.

[Andrei Sekretenko]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

Added unit tests for 'framework::validateUpdate()'.


Diffs
-

  src/tests/master_validation_tests.cpp 
1b7a8273f2704e0a4dedf33c55ede33dc1b1a4af 


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


Testing
---


Thanks,

Andrei Sekretenko

Review Request 70666: Introduced a function for validating a `FrameworkInfo` update.

[Andrei Sekretenko]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

This patch moves the code validating a new `FrameworkInfo` against
the current one into a separate function.


Diffs
-

  src/master/master.cpp c72b92656dddca63ee89abf676da66bd76f58a6d 
  src/master/validation.hpp 95638a17052ece6c957aa76e4cead8d7bfe82024 
  src/master/validation.cpp 9fb0850987ce385d345302cac9721adead7181b8 


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


Testing
---


Thanks,

Andrei Sekretenko

Review Request 70665: Moved the logic of sending 'Framework' updates into a separate method.

[Andrei Sekretenko]

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

Review request for mesos.


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


Repository: mesos


Description
---

This patch consolidates the scattered logic of sending Framework updates
(namely, the FRAMEWORK_ADDED event to master V1 API subscribers and
UpdateFrameworkMessage to agents).

NOTE: The UpdateFrameworkMessage is now also being sent in the case of
non-`forced` subscription request from a framework which has already
been registered. Previously it was not being sent in this case -
this likely is a bug.


Diffs
-

  src/master/master.hpp c523c937e294eaffe0e58306c267770c119c9f42 
  src/master/master.cpp c72b92656dddca63ee89abf676da66bd76f58a6d 


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


Testing
---


Thanks,

Andrei Sekretenko

Review Request 70664: Made `activateRecoveredFramework()` return void instead of Nothing().

[Andrei Sekretenko]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

The `Master::activateRecoveredFramework()` method always returns
`Nothing()`. This patch changes its return type to `void` and removes
the unused error handling code.


Diffs
-

  src/master/master.hpp c523c937e294eaffe0e58306c267770c119c9f42 
  src/master/master.cpp c72b92656dddca63ee89abf676da66bd76f58a6d 


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


Testing
---


Thanks,

Andrei Sekretenko

Review Request 70663: Removed non-implemented declaration of 'Master::validate()'.

[Andrei Sekretenko]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

Removed non-implemented declaration of 'Master::validate()'.


Diffs
-

  src/master/master.hpp c523c937e294eaffe0e58306c267770c119c9f42 


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


Testing
---


Thanks,

Andrei Sekretenko

Re: Review Request 70532: Added an UPDATE_FRAMEWORK scheduler::Call.

[Andrei Sekretenko]

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

(Updated May 17, 2019, 3 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Now the new call type is added to the switch statements in this patch.


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


Repository: mesos


Description
---

This patch definies the call to be used by a framework for updating its
FrameworkInfo fields without re-subscribing.

It is based on the previous implementation attempt:
https://reviews.apache.org/r/66228/


Diffs (updated)
-

  include/mesos/scheduler/scheduler.proto 
b6d79b1c433dce77a6ee4278b3ddf6b69868c1d8 
  include/mesos/v1/scheduler/scheduler.proto 
bddd5c449fd4b294d15430746b708731aff18f2a 
  src/java/jni/org_apache_mesos_v1_scheduler_V0Mesos.cpp 
35a04a3d0183697f93dadb3cbfead3ee0c2fea08 
  src/master/http.cpp c2c7b9b65bd66679420f62d37b01d90e3692c71d 
  src/master/master.cpp c72b92656dddca63ee89abf676da66bd76f58a6d 
  src/master/validation.cpp 9fb0850987ce385d345302cac9721adead7181b8 


Diff: https://reviews.apache.org/r/70532/diff/6/

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


Testing
---


Thanks,

Andrei Sekretenko

Re: Review Request 70573: Updated bintray upload scripts to remove hard-coded accounts.

[Joseph Wu]

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


Ship it!




Looks sane.  

And I see that any process changes will be documented here: 
https://reviews.apache.org/r/70528/

- Joseph Wu


On May 3, 2019, 12:48 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70573/
> ---
> 
> (Updated May 3, 2019, 12:48 p.m.)
> 
> 
> Review request for mesos and Till Toenshoff.
> 
> 
> Bugs: MESOS-9697
> https://issues.apache.org/jira/browse/MESOS-9697
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This updates the Jenkinsfile used in the ASF Jenkins as well
> as the associated upload script for bintray to make the
> used credential id configurable and to upload the built packages
> to the official `apache/mesos` bintray account.
> 
> 
> Diffs
> -
> 
>   support/packaging/Jenkinsfile 7446b2b8b88df6c146f4415b06dda227ca8ce631 
>   support/packaging/bintray.sh 4f1795f14b078e8eebe95aaa40cf860d6d19ee97 
> 
> 
> Diff: https://reviews.apache.org/r/70573/diff/1/
> 
> 
> Testing
> ---
> 
> Not sure how to test jenkins pipelines :/
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 67177: Sorted container mounts by their target paths.

[James Peach]

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



I don't think I have enough context here. Can you describe the prblem this is 
solving in more detail? Why do we need to sort by source path at all?

- James Peach


On May 17, 2018, 1:28 a.m., Jason Lai wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67177/
> ---
> 
> (Updated May 17, 2018, 1:28 a.m.)
> 
> 
> Review request for mesos, Eric Chung, Gilbert Song, Jie Yu, James Peach, and 
> Zhitao Li.
> 
> 
> Bugs: MESOS-6798
> https://issues.apache.org/jira/browse/MESOS-6798
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Container mounts are sorted by:
> 1) their target paths, and then
> 2) their source paths,
> before mounting them upon container launch.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/launch.cpp 
> f25d90651ef32495c9161c3eaed8a327d1b2b926 
> 
> 
> Diff: https://reviews.apache.org/r/67177/diff/1/
> 
> 
> Testing
> ---
> 
> Manual
> 
> 
> Thanks,
> 
> Jason Lai
> 
>

Re: Review Request 70661: Added `CHECK_CONTAINS` and `CHECK_NOT_CONTAINS` macros.

[Benjamin Mahler]

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


Ship it!




- Benjamin Mahler


On May 17, 2019, 6:46 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70661/
> ---
> 
> (Updated May 17, 2019, 6:46 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Meng Zhu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a simple `CHECK_CONTAINS` and `CHECK_NOT_CONTAINS` to replace
> checks of the form `CHECK(foo.contains(bar))`. These macros format
> the value of the key in their failure message to assist debugging.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/check.hpp 
> 7651150bb6cbe148af1e148658deff3d65ae4746 
> 
> 
> Diff: https://reviews.apache.org/r/70661/diff/1/
> 
> 
> Testing
> ---
> 
> make check (Fedora 30)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 70651: Notifies master `/api/v1` subscribers about recovered frameworks.

[Greg Mann]

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


Fix it, then Ship it!





src/tests/api_tests.cpp
Lines 2618 (patched)


s/notified/broadcast to subscribers/



src/tests/api_tests.cpp
Lines 2774 (patched)


Nit: make this a const ref?



src/tests/api_tests.cpp
Lines 2781 (patched)


Nit: const ref?



src/tests/api_tests.cpp
Line 2782 (original), 2792 (patched)


Nit: const ref?


- Greg Mann


On May 16, 2019, 5:11 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70651/
> ---
> 
> (Updated May 16, 2019, 5:11 a.m.)
> 
> 
> Review request for mesos and Greg Mann.
> 
> 
> Bugs: MESOS-9785
> https://issues.apache.org/jira/browse/MESOS-9785
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If one subscribes to master's `/api/v1` endpoint after a master failover
> but before an agent reregistration, frameworks recovered through the
> agent registration should be notified to the subscriber, otherwise
> recovered tasks will have framework IDs referring to frameworks unknown
> to the subscriber.
> 
> 
> Diffs
> -
> 
>   src/common/protobuf_utils.cpp 8b252cb11e17356836988dfc44a63953579a1def 
>   src/master/master.cpp 6c0e30bff3fbfe0ad1f3af44eb3df6f82048f0e1 
>   src/tests/api_tests.cpp bc19d7e9661f091aebf3072967548c7c3196239d 
> 
> 
> Diff: https://reviews.apache.org/r/70651/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 70637: Updated CHECK messages in the heirarchical allocator.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [70661, 70637]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose --disable-libtool-wrappers 
--disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker-build.sh

- Mesos Reviewbot


On May 16, 2019, 11:47 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70637/
> ---
> 
> (Updated May 16, 2019, 11:47 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Meng Zhu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The hierarchical allocator a number of has CHECKs that enforce
> the invariants about the state of roles, agents and frameworks.
> This change adds the relevant IDs to the CHECK message so that
> it is easier to track down the root cause of failures.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 875cfcd091f5f58afb89e752da5100a75828dd67 
> 
> 
> Diff: https://reviews.apache.org/r/70637/diff/2/
> 
> 
> Testing
> ---
> 
> make check (Fedora 30)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Review Request 70661: Added `CHECK_CONTAINS` and `CHECK_NOT_CONTAINS` macros.

[James Peach]

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

Review request for mesos, Benjamin Mahler and Meng Zhu.


Repository: mesos


Description
---

Added a simple `CHECK_CONTAINS` and `CHECK_NOT_CONTAINS` to replace
checks of the form `CHECK(foo.contains(bar))`. These macros format
the value of the key in their failure message to assist debugging.


Diffs
-

  3rdparty/stout/include/stout/check.hpp 
7651150bb6cbe148af1e148658deff3d65ae4746 


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


Testing
---

make check (Fedora 30)


Thanks,

James Peach

Re: Review Request 70637: Updated CHECK messages in the heirarchical allocator.

[James Peach]

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

(Updated May 17, 2019, 6:47 a.m.)


Review request for mesos, Benjamin Mahler and Meng Zhu.


Repository: mesos


Description
---

The hierarchical allocator a number of has CHECKs that enforce
the invariants about the state of roles, agents and frameworks.
This change adds the relevant IDs to the CHECK message so that
it is easier to track down the root cause of failures.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.cpp 
875cfcd091f5f58afb89e752da5100a75828dd67 


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

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


Testing
---

make check (Fedora 30)


Thanks,

James Peach