Re: Review Request 64095: Added a generic actor to be used by status update managers.

[Greg Mann]

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




src/status_update_manager/status_update_manager_process.hpp
Lines 56-58 (patched)


Should we add "Recovering status update state after failover" to this list?

Also, should we explicitly say that the SUM is not responsible for garbage 
collection of completed streams?



src/status_update_manager/status_update_manager_process.hpp
Lines 67 (patched)


After looking at the recovery code, looks like we should replace: 
s/immediately after recovery/during recovery/



src/status_update_manager/status_update_manager_process.hpp
Lines 73-74 (patched)


I did a quick search and it looks like we usually indent 4 spaces for 
template parameters? Could you confirm and update if appropriate?



src/status_update_manager/status_update_manager_process.hpp
Lines 95 (patched)


Add a comment explaining this member?



src/status_update_manager/status_update_manager_process.hpp
Lines 110-111 (patched)


Place on same line as initialization list.



src/status_update_manager/status_update_manager_process.hpp
Lines 115-120 (patched)


Is this code necessary?



src/status_update_manager/status_update_manager_process.hpp
Lines 128 (patched)


s/a call-back//



src/status_update_manager/status_update_manager_process.hpp
Lines 152 (patched)


Is this the appropriate place for this comment? Perhaps it can be removed?



src/status_update_manager/status_update_manager_process.hpp
Lines 182 (patched)


s/frameworkId/`frameworkId`/

or

s/frameworkId/framework ID/



src/status_update_manager/status_update_manager_process.hpp
Lines 206 (patched)


I don't understand this comment?



src/status_update_manager/status_update_manager_process.hpp
Lines 227 (patched)


Backticks instead of quotes for consistency.



src/status_update_manager/status_update_manager_process.hpp
Lines 237-238 (patched)


Maybe we should leave a TODO either here or next to the constant 
declaration saying that we should move this constant into this file once 
MESOS-8296 is resolved?



src/status_update_manager/status_update_manager_process.hpp
Lines 268 (patched)


s/the status update stream for stream/status update stream/



src/status_update_manager/status_update_manager_process.hpp
Lines 268-269 (patched)


Indent 4 spaces.



src/status_update_manager/status_update_manager_process.hpp
Lines 285 (patched)


Can probably remove this comment.



src/status_update_manager/status_update_manager_process.hpp
Lines 311 (patched)


Period.



src/status_update_manager/status_update_manager_process.hpp
Lines 331-332 (patched)


The following is more consistent with our style guide:

```
const std::string message =
  "Failed to recover status update stream " +
  stringify(streamId) + ": " + result.error();
```



src/status_update_manager/status_update_manager_process.hpp
Lines 405 (patched)


Are we sure that this is being resent? Isn't it possible that the update 
was queued while the manager was paused?



src/status_update_manager/status_update_manager_process.hpp
Lines 429-432 (patched)


4 Space indent



src/status_update_manager/status_update_manager_process.hpp
Lines 431 (patched)


Let's not use the C-style cast here.

How about:
```
checkpoint ? Option(getPath(streamId)) : None());
```

I think you'll need to use `Option::none()` for the NONE if 
the compiler complains.



src/status_update_manager/status_update_manager_process.hpp
Lines 454-457 (patched)


This indentation is difficult to parse.

How about
```
Result> 

Re: Review Request 63387: Added publish/unpublish in storage local resource provider.

[Chun-Hung Hsiao]

> On Dec. 6, 2017, 12:23 a.m., Jie Yu wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 696-698 (patched)
> > 
> >
> > Can you comment on why this is not a fatal error and it is safe to 
> > ignore?

I'll follow the error handling we have for `ResourceProviderState` 
checkpointing.


> On Dec. 6, 2017, 12:23 a.m., Jie Yu wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 749-752 (patched)
> > 
> >
> > Why do you need this? Can you add a `default` and set it to UNREACHABLE?

https://github.com/google/protobuf/issues/3917


> On Dec. 6, 2017, 12:23 a.m., Jie Yu wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 962-965 (patched)
> > 
> >
> > Can this just be `default`?

Ditto.


> On Dec. 6, 2017, 12:23 a.m., Jie Yu wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 1578-1582 (patched)
> > 
> >
> > Do you need this?

No I don't need. Missed this during refactoring :(


- Chun-Hung


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


On Dec. 5, 2017, 5:59 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63387/
> ---
> 
> (Updated Dec. 5, 2017, 5:59 a.m.)
> 
> 
> Review request for mesos, James DeFelice, Jie Yu, Joseph Wu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-8143
> https://issues.apache.org/jira/browse/MESOS-8143
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Storage local resource provider can now handle PUBLISH events.
> Although we don't do UNPUBLISH for now, the unpublish function is
> still required for deleting volumes.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am d5ca797c5a57a2763374d391a7e9358fe29f5e06 
>   src/csi/state.hpp PRE-CREATION 
>   src/csi/state.proto PRE-CREATION 
>   src/csi/utils.hpp 54cf34b40bd48282bbe3f8bcfe664540d9ce79b5 
>   src/csi/utils.cpp 590e5f4f763734748932584e383287bf94a5ec18 
>   src/resource_provider/storage/provider.cpp 
> d35b0d02992e3730ca47906b34c21e1ba9c653e7 
> 
> 
> Diff: https://reviews.apache.org/r/63387/diff/6/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 61128: Improved log messages in master when adding/removing tasks/executors.

[Vinod Kone]

> On Nov. 7, 2017, 12:37 a.m., Qian Zhang wrote:
> > src/master/master.cpp
> > Lines 9674-9676 (original), 9675-9678 (patched)
> > 
> >
> > To be consistent with this log message, I think we also need to change 
> > the log message in `Master::removeTask()` from:
> > ```
> > LOG(INFO) << "Removing task " << task->task_id()
> >   << " with resources " << task->resources()
> >   << " of framework " << task->framework_id()
> >   << " on agent " << *slave;
> > ```
> > to:
> > ```
> > LOG(INFO) << "Removing task " << task->task_id()
> >   << " of framework " << task->framework_id()
> >   << " with resources " << task->resources()
> >   << " on agent " << *slave;
> > ```
> > And there is also a warning message in `Master::removeTask()`, I think 
> > we need to change that one too.

Made it consistent. Also moved the location of the logs to be consistent.


- Vinod


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


On Dec. 6, 2017, 1:33 a.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61128/
> ---
> 
> (Updated Dec. 6, 2017, 1:33 a.m.)
> 
> 
> Review request for mesos, Benno Evers and Qian Zhang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made the log messages and the calling sites consistent and also added
> one for adding an executor.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 16cdde73b90b2e1514c6196850b8df75a3b6ff28 
> 
> 
> Diff: https://reviews.apache.org/r/61128/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 61128: Improved log messages in master when adding/removing tasks/executors.

[Vinod Kone]

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

(Updated Dec. 6, 2017, 1:33 a.m.)


Review request for mesos, Benno Evers and Qian Zhang.


Changes
---

qian's comments.


Repository: mesos


Description (updated)
---

Made the log messages and the calling sites consistent and also added
one for adding an executor.


Diffs (updated)
-

  src/master/master.cpp 16cdde73b90b2e1514c6196850b8df75a3b6ff28 


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

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


Testing (updated)
---

make check


Thanks,

Vinod Kone

Review Request 64357: Updated the allocator to untrack allocations via a single code path.

[Meng Zhu]

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

Review request for mesos and Benjamin Mahler.


Repository: mesos


Description
---

Introduced a helper for untracking allocated resources in the
sorters.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.hpp 
0622026858f7cff87ff0086b7be65f30c725 
  src/master/allocator/mesos/hierarchical.cpp 
d3485160df0c3c2fceba551371a5cade81bb4c71 


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


Testing
---

make check


Thanks,

Meng Zhu

Re: Review Request 63388: Handling offer operations in storage local resource provider.

[Jie Yu]

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




src/resource_provider/storage/provider.cpp
Lines 1901-1931 (patched)


See my other comments. Let's move the resource generation/conversion part 
into a separate helper and make `createVolume` a pure CSI operation.



src/resource_provider/storage/provider.cpp
Lines 1947-1949 (patched)


Hum, i like to keep `deleteVolume` helper simple to just CSI related 
operations (like you did for publich/unpublish related operations).

The generation of the resources conversion should be in 
`applyOfferOperation`.

Ditto for `createVolume`



src/resource_provider/storage/provider.cpp
Lines 1971 (patched)


typo?



src/resource_provider/storage/provider.cpp
Lines 2038 (patched)


using iterator in the interface is something we never do in other places. 
I'd suggest we keep track of a hashmap with key being the operation uuid.


- Jie Yu


On Dec. 5, 2017, 2:19 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63388/
> ---
> 
> (Updated Dec. 5, 2017, 2:19 a.m.)
> 
> 
> Review request for mesos, James DeFelice, Jie Yu, Joseph Wu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-8108
> https://issues.apache.org/jira/browse/MESOS-8108
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> For legacy operations, we just call Resources::apply(). New operations
> CREATE_VOLUME, DESTROY_VOLUME, CREATE_BLOCK, DESTROY_BLOCK are
> implemented through CSI. Specially, DESTROY_* requires unpublishing
> the resources first.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> d35b0d02992e3730ca47906b34c21e1ba9c653e7 
> 
> 
> Diff: https://reviews.apache.org/r/63388/diff/5/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 63387: Added publish/unpublish in storage local resource provider.

[Jie Yu]

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


Fix it, then Ship it!





src/resource_provider/storage/provider.cpp
Lines 310 (patched)


Can you add a comment about why `sequence` is needed?



src/resource_provider/storage/provider.cpp
Lines 668 (patched)


attach the failure?



src/resource_provider/storage/provider.cpp
Lines 696-698 (patched)


Can you comment on why this is not a fatal error and it is safe to ignore?



src/resource_provider/storage/provider.cpp
Lines 749-752 (patched)


Why do you need this? Can you add a `default` and set it to UNREACHABLE?



src/resource_provider/storage/provider.cpp
Lines 932-933 (patched)


What if there are multiple resources (e.g, PATH) disk that points to the 
same volume id? Can you add some comments about that?



src/resource_provider/storage/provider.cpp
Lines 933 (patched)


Can you add a CHECK here asserting that the resource is a disk resource 
with an ID?



src/resource_provider/storage/provider.cpp
Lines 962-965 (patched)


Can this just be `default`?



src/resource_provider/storage/provider.cpp
Lines 1578-1582 (patched)


Do you need this?



src/resource_provider/storage/provider.cpp
Lines 1636-1640 (patched)


DO you need this?



src/resource_provider/storage/provider.cpp
Lines 1704-1708 (patched)


Do you need this?


- Jie Yu


On Dec. 5, 2017, 5:59 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63387/
> ---
> 
> (Updated Dec. 5, 2017, 5:59 a.m.)
> 
> 
> Review request for mesos, James DeFelice, Jie Yu, Joseph Wu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-8143
> https://issues.apache.org/jira/browse/MESOS-8143
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Storage local resource provider can now handle PUBLISH events.
> Although we don't do UNPUBLISH for now, the unpublish function is
> still required for deleting volumes.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am d5ca797c5a57a2763374d391a7e9358fe29f5e06 
>   src/csi/state.hpp PRE-CREATION 
>   src/csi/state.proto PRE-CREATION 
>   src/csi/utils.hpp 54cf34b40bd48282bbe3f8bcfe664540d9ce79b5 
>   src/csi/utils.cpp 590e5f4f763734748932584e383287bf94a5ec18 
>   src/resource_provider/storage/provider.cpp 
> d35b0d02992e3730ca47906b34c21e1ba9c653e7 
> 
> 
> Diff: https://reviews.apache.org/r/63387/diff/6/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 64145: Added status update acknowledgement to resource provider manager.

[Gaston Kleiman]

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


Ship it!




Ship It!

- Gaston Kleiman


On Dec. 4, 2017, 11:37 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64145/
> ---
> 
> (Updated Dec. 4, 2017, 11:37 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Jie Yu.
> 
> 
> Bugs: MESOS-8186
> https://issues.apache.org/jira/browse/MESOS-8186
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When the agent receives an offer operation update acknowledgement
> from the master, it is forwarded to the relevant resource
> provider via the resource provider manager.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/manager.hpp c2aeb15b0b8ebd167ddf9c42c9fc396d4c0a126b 
>   src/resource_provider/manager.cpp 8d8b2f1396230d6edba590b8b7e9b4ca51366efe 
>   src/resource_provider/storage/provider.cpp 
> d35b0d02992e3730ca47906b34c21e1ba9c653e7 
> 
> 
> Diff: https://reviews.apache.org/r/64145/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 64143: Added ACKNOWLEDGE event to the resource provider API.

[Gaston Kleiman]

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


Ship it!




Ship It!

- Gaston Kleiman


On Dec. 4, 2017, 11:33 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64143/
> ---
> 
> (Updated Dec. 4, 2017, 11:33 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Jie Yu.
> 
> 
> Bugs: MESOS-8186
> https://issues.apache.org/jira/browse/MESOS-8186
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The new event is used to send offer operation status udpate
> acknowledgements to resource providers.
> 
> 
> Diffs
> -
> 
>   include/mesos/resource_provider/resource_provider.proto 
> 2ce71f4c3e97e89c226966c347c2817e2bab9c1b 
>   include/mesos/v1/resource_provider/resource_provider.proto 
> 465b11dafd7c2e3b9476696ed75c1c077d6c8eeb 
>   src/resource_provider/storage/provider.cpp 
> d35b0d02992e3730ca47906b34c21e1ba9c653e7 
>   src/tests/mesos.hpp 3a9b1fbd4301c6cccbc770329ff71ed8ff7e86a2 
> 
> 
> Diff: https://reviews.apache.org/r/64143/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 64142: Updated master ACCEPT handler to disallow offer operation feedback.

[Gaston Kleiman]

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


Ship it!




Ship It!

- Gaston Kleiman


On Dec. 4, 2017, 2:08 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64142/
> ---
> 
> (Updated Dec. 4, 2017, 2:08 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gaston Kleiman, Jie Yu, and Jan 
> Schlicht.
> 
> 
> Bugs: MESOS-8054
> https://issues.apache.org/jira/browse/MESOS-8054
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch updates the master's ACCEPT call code path to fail
> offer operations when their `id` field is set. Since protobufs
> have already been updated for offer operation feedback, but the
> feature is not fully implemented, we will disallow the setting
> of this field for now.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 883793a226849434eb833c3027d121635a86fdc4 
> 
> 
> Diff: https://reviews.apache.org/r/64142/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 64093: Added operators for offer operation update protobuf classes.

[Gaston Kleiman]

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

(Updated Dec. 5, 2017, 3:47 p.m.)


Review request for mesos and Greg Mann.


Changes
---

MaDe capitalization consistent.


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


Repository: mesos


Description
---

Added operators for offer operation update protobuf classes.


Diffs (updated)
-

  include/mesos/type_utils.hpp b786d0e1b887306e68561ce7c9907fa844d5786a 
  src/common/type_utils.cpp 1b466fcfd133b6fd0067668c7ba54d1ce80c8702 
  src/messages/messages.hpp 2756ebae5e2dccce63030eb7cec55d35445fa97b 
  src/messages/messages.cpp 6029502e8394817a0ab6ee2f63895ad02349bb0d 


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

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


Testing
---

Added new tests at the end of the chain. They passed 5000 times on GNU/Linux.


Thanks,

Gaston Kleiman

Re: Review Request 63022: Imported and reconcile resources from CSI plugins.

[Jie Yu]

> On Dec. 5, 2017, 2:08 p.m., James DeFelice wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 1203 (patched)
> > 
> >
> > some plugins might not implement create/delete, but DO implement list. 
> > such a plugin might only support "block"- (or only support "mount"-) type 
> > volumes. in such cases, it could be useful to report those resources as 
> > BLOCK (or MOUNT) volumes instead of RAW. I don't *think* it's a blocker for 
> > MVP, but we've had a couple of conversations recently about exposing 
> > low-level primitives to specialized storage frameworks, and that it might 
> > be useful to report block devices as BLOCK disk resources, by default.

The framework should decide whether to convert the RAW into either MOUNT or 
BLOCK based on how it intends to use the volume.

This does raise one valid case for default profile. Currently, the default 
profile will call `ValidateVolumeCapabilities` using `MountVolume` capability. 
It does not test for if the volume supports BlockVolume or not. That mean for 
pre-existing volume, CreateVolume/CreateBlock handler will need to call 
`ValidateVolumeCapabilities` before it converts the resources to either 
MOUNT/PATH, or BLOCK. That makes me wonder if we need to call 
`ValidateVolumeCapabilities` at all for volumes that does not have a profile.


- Jie


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


On Dec. 5, 2017, 5:57 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63022/
> ---
> 
> (Updated Dec. 5, 2017, 5:57 a.m.)
> 
> 
> Review request for mesos, James DeFelice, Jie Yu, Joseph Wu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-8101
> https://issues.apache.org/jira/browse/MESOS-8101
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The following lists the steps to reconcile resources:
> 
> 1. Import resources from the CSI plugin:
>   a. Get preprovisioned volumes:
>ListVolumes
>ValidateVolumeCapabilities for each volume
>   b. GetCapacity for each profile to get RAW resources
> 2. For each resource in the checkpointed resources:
>   a. Strip away metadata (reservation, persistence, etc)
>   b. Additional resources after subtracting the stripped resources
>  from the imported resources are new resources.
> 3. Report the reconciled resources through UPDATE_STATE
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> d35b0d02992e3730ca47906b34c21e1ba9c653e7 
> 
> 
> Diff: https://reviews.apache.org/r/63022/diff/16/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 64304: Enforced quota limit in the presence of unallocated reservations.

[Meng Zhu]

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

(Updated Dec. 5, 2017, 2:42 p.m.)


Review request for mesos, Alexander Rukletsov, Benjamin Mahler, and Michael 
Park.


Changes
---

Patch updated. Thanks for the review!


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


Repository: mesos


Description
---

Enforced quota limit in the presence of unallocated reservations.
Also modify the allocation process such that the first stage allocation
is solely for quota roles while the second stage is solely for
non-quota roles.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.cpp 
4bc9fb6f787224114f1285937cf979ace46d8ea3 
  src/tests/hierarchical_allocator_tests.cpp 
154049c47f2dae2df8d1bb4ddb5c48c478bb3d0e 


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

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


Testing (updated)
---

Introduced two dedicated tests.
make check.


Thanks,

Meng Zhu

Re: Review Request 64304: Enforced quota limit in the presence of unallocated reservations.

[Meng Zhu]

> On Dec. 4, 2017, 5:08 p.m., Benjamin Mahler wrote:
> > src/tests/hierarchical_allocator_tests.cpp
> > Lines 1272 (patched)
> > 
> >
> > After reading through the test, this is the dynamic reservation case 
> > only? Should we call this test `QuotaLimitWithDynamicReservation` and have 
> > another test for static reservations? Or do you want to test both cases in 
> > this test?

Tests parameterized.


> On Dec. 4, 2017, 5:08 p.m., Benjamin Mahler wrote:
> > src/tests/hierarchical_allocator_tests.cpp
> > Lines 1363 (patched)
> > 
> >
> > This test is also just dynamic reservations, so the same comment above 
> > applies. Maybe we need to parameterize these tests based on whether to use 
> > static or dynamic reservation?
> > 
> > It's also not clear to me why we need both tests, instead of just a 
> > single test.
> > 
> > E.g.
> > 
> > quota = R
> > add agent 1 with R resources, reserve them, decline forever
> > add agent 2 with R resources, they should not be allocated
> > 
> > revive
> > 
> > should only get agent 1 resources offered
> > trigger another allocation cycle (to make sure it's enforced across 
> > cycles)
> > agent 2 should not be offered to the role

Parameterized the test for both dynamic reservation and static reservation.

The first test ensures that quota limit does not get exceeded in the presence 
of unallocated-reservation. (expect no pending allocation)

The second test ensures that when a role’s reserved resources gets allocated 
and (by that time) if its quota still has not been fully satisfied, it can get 
unreserved resources to meet its quota. (expect allocation of unreserved 
resources).

I do not think you proposed test checks that. I clarified the comments and test 
name.


- Meng


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


On Dec. 5, 2017, 2:42 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64304/
> ---
> 
> (Updated Dec. 5, 2017, 2:42 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Mahler, and Michael 
> Park.
> 
> 
> Bugs: MESOS-4527
> https://issues.apache.org/jira/browse/MESOS-4527
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Enforced quota limit in the presence of unallocated reservations.
> Also modify the allocation process such that the first stage allocation
> is solely for quota roles while the second stage is solely for
> non-quota roles.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 4bc9fb6f787224114f1285937cf979ace46d8ea3 
>   src/tests/hierarchical_allocator_tests.cpp 
> 154049c47f2dae2df8d1bb4ddb5c48c478bb3d0e 
> 
> 
> Diff: https://reviews.apache.org/r/64304/diff/4/
> 
> 
> Testing
> ---
> 
> Introduced two dedicated tests.
> make check.
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 63022: Imported and reconcile resources from CSI plugins.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Dec. 5, 2017, 5:57 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63022/
> ---
> 
> (Updated Dec. 5, 2017, 5:57 a.m.)
> 
> 
> Review request for mesos, James DeFelice, Jie Yu, Joseph Wu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-8101
> https://issues.apache.org/jira/browse/MESOS-8101
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The following lists the steps to reconcile resources:
> 
> 1. Import resources from the CSI plugin:
>   a. Get preprovisioned volumes:
>ListVolumes
>ValidateVolumeCapabilities for each volume
>   b. GetCapacity for each profile to get RAW resources
> 2. For each resource in the checkpointed resources:
>   a. Strip away metadata (reservation, persistence, etc)
>   b. Additional resources after subtracting the stripped resources
>  from the imported resources are new resources.
> 3. Report the reconciled resources through UPDATE_STATE
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> d35b0d02992e3730ca47906b34c21e1ba9c653e7 
> 
> 
> Diff: https://reviews.apache.org/r/63022/diff/16/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 64355: Fixed a flaky test.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Dec. 5, 2017, 10:37 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64355/
> ---
> 
> (Updated Dec. 5, 2017, 10:37 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-8288
> https://issues.apache.org/jira/browse/MESOS-8288
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This fixes SlaveTest.IgnoreV0ExecutorIfItReregistersWithoutReconnect
> to expect the shutdown to occur at most once, since it's possible
> that when the test objects are being destructed, the executor's
> shutdown method is not called.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_tests.cpp eee6aa4cfb607b7c46a6d44e8893649110c1c8da 
> 
> 
> Diff: https://reviews.apache.org/r/64355/diff/1/
> 
> 
> Testing
> ---
> 
> Ran in repetition.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Review Request 64355: Fixed a flaky test.

[Benjamin Mahler]

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

Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

This fixes SlaveTest.IgnoreV0ExecutorIfItReregistersWithoutReconnect
to expect the shutdown to occur at most once, since it's possible
that when the test objects are being destructed, the executor's
shutdown method is not called.


Diffs
-

  src/tests/slave_tests.cpp eee6aa4cfb607b7c46a6d44e8893649110c1c8da 


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


Testing
---

Ran in repetition.


Thanks,

Benjamin Mahler

Review Request 64354: Added tests for UriVolumeProfile module.

[Joseph Wu]

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

Review request for mesos, Chun-Hung Hsiao and Jie Yu.


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


Repository: mesos


Description
---

These tests mostly exercise the parsing logic used by the module
and some of the interaction expected from callers of the module
(i.e. the Storage Local Resource Provider).


Diffs
-

  src/Makefile.am 05e8b950a3ee13f7b2e8af9416495f2827138449 
  src/tests/volume_profile_tests.cpp PRE-CREATION 


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


Testing
---

make check


Thanks,

Joseph Wu

Review Request 64353: Added example VolumeProfile module.

[Joseph Wu]

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

Review request for mesos, Chun-Hung Hsiao and Jie Yu.


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


Repository: mesos


Description
---

This example module shows how a VolumeProfile module might be
implemented (and is a viable module in its own right).  The module
can be configured to fetch a map of profiles from a URI (`file://` or
`http(s)://`) and possibly cache this item for some time.


Diffs
-

  src/Makefile.am 05e8b950a3ee13f7b2e8af9416495f2827138449 
  src/resource_provider/uri_volume_profile.hpp PRE-CREATION 
  src/resource_provider/uri_volume_profile.cpp PRE-CREATION 


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


Testing
---

See end of chain.


Thanks,

Joseph Wu

Re: Review Request 64098: Send status updates when agent re-registers.

[Jiang Yan Xu]

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



Awesome! Only minor comments below.


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






src/master/master.cpp
Lines 6806-6810 (patched)


The fact that an unknown agent is allowed to register is already logged: 
https://github.com/apache/mesos/blob/c9462f4927cfffb1f3a90827467ded730c0f40b9/src/master/registry_operations.cpp#L133

Here I was orignally thinking we needed to add some information about the 
fact that the update is for an unknown agent. However since the logging in 
`forward` already logs `update.status().message()`, perhaps it's fine and so we 
don't need a separate log line after all. :)



src/master/master.cpp
Lines 6812-6814 (patched)


Our convention if the two conditions are on separate lines:

```
const string message = slaves.unreachable.contains(slaveInfo.id())
? "Unreachable agent re-reregistered"
: "Unknown agent reregistered";
```

Note that I s/re-registration/reregistered/ because it's more consistent 
with other status messages.



src/master/master.cpp
Line 6809 (original), 6816 (patched)


Should this line be the first thing done under the if condition since the 
rest of the logic is about sending status updates?



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


So we don't log anything when `framework == nullptr` but log a warning when 
`!framework->connected()`, should we treat the two cases the same?



src/master/master.cpp
Lines 6834-6835 (patched)


Let's log the message of the update as well (the same way `forward` does 
it).

FWIW I think it's probably the best to log it in `ostream& 
operator<<(ostream& stream, const StatusUpdate& update)` but it probably 
requires a separate commit to refactor.



src/master/master.cpp
Lines 6842-6843 (patched)


This comment comes from 
https://github.com/apache/mesos/commit/678b864cb78c74cc98b960f921d07869ce3300c5 
which I don't think is relevant anymore and can be removed.



src/tests/master_allocator_tests.cpp
Lines 1362 (patched)


Not yours but the use of `this->` is annoying here (we generally don't do 
it unless for disambiguation) so perhaps don't add more occurrences for now and 
keep the existing ocurrences for a future sweep.



src/tests/master_tests.cpp
Lines 2881-2883 (original), 2885-2887 (patched)


Can we also set the expectation for the new reason?



src/tests/master_tests.cpp
Lines 7188-7190 (patched)


You have put the expectations above `detector.appoint(master.get()->pid);` 
in the test above. 

I think it's a better and clearer alternative to prevent race conditions. 

Even though there's a 
`Clock::advance(agentFlags.registration_backoff_factor);` below to prevent it 
when the clock is paused, it's clearer and more foolproof if we don't insert 
the expectation between `detector.appoint(master.get()->pid);` and the clock 
advancement.

Plus 

```
detector.appoint(master.get()->pid);
Clock::advance(agentFlags.registration_backoff_factor);
```

can be thought of as the same group of statements so better to be kept 
together.

Here and below.



src/tests/master_tests.cpp
Lines 7198-7199 (patched)


Can we also set the expectation on the reason?



src/tests/master_tests.cpp
Lines 7661-7663 (patched)


Ditto on pulling it above `detector.appoint(master.get()->pid);`.



src/tests/master_tests.cpp
Lines 7669-7671 (patched)


Can we also set the expectation on the reason?

Can we do this for other places (when reasonable)?



src/tests/partition_tests.cpp
Line 2181 (original), 2146 (patched)


How come the commit hook didn't catch this over 80 char line?



src/tests/partition_tests.cpp
Lines 2299 (patched)


Here it seems that we'll receive two status updates, one from the agent and 
one sent by the master.

Setting `WillRepeatedly` here is probably racy due to 

```
EXPECT_CALL(sched, statusUpdate(, _))

Re: Review Request 63761: Replaced std::shared_ptr with std::unique_ptr in dispatch.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Dec. 5, 2017, 10:07 a.m., Dmitry Zhuk wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63761/
> ---
> 
> (Updated Dec. 5, 2017, 10:07 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Michael Park.
> 
> 
> Bugs: MESOS-6972
> https://issues.apache.org/jira/browse/MESOS-6972
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since `dispatch` can now handle movable only parameters, `Promise` and
> functor can be safely wrapped into `std::unique_ptr` for efficiency.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/dispatch.hpp 
> 155f362a888caef5ce2db803a109400ca4c7ec37 
>   3rdparty/libprocess/include/process/event.hpp 
> 571ffb1ea5c4787f10c2f4dee249aadc8b904bc6 
>   3rdparty/libprocess/src/process.cpp 
> 64bcce215d19558dd493e30e96ca16577fe0722a 
> 
> 
> Diff: https://reviews.apache.org/r/63761/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Dmitry Zhuk
> 
>

Re: Review Request 63913: Replaced std::shared_ptr with std::unique_ptr in Future.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Nov. 21, 2017, 9:52 a.m., Dmitry Zhuk wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63913/
> ---
> 
> (Updated Nov. 21, 2017, 9:52 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Michael Park.
> 
> 
> Bugs: MESOS-6972
> https://issues.apache.org/jira/browse/MESOS-6972
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Replaced std::shared_ptr with std::unique_ptr in Future.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/future.hpp 
> d48dfd795fdc4021270621522a365e3cad4102a6 
> 
> 
> Diff: https://reviews.apache.org/r/63913/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Dmitry Zhuk
> 
>

Review Request 64352: Added default VolumeProfile module implementation.

[Joseph Wu]

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

Review request for mesos, Chun-Hung Hsiao and Jie Yu.


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


Repository: mesos


Description
---

By default, Mesos and the Storage Local Resource Provider (SLRP) will
not expect profiles to be used. This is partly due to the experimental
nature of the "profile" field and partly because some explicit operator
intervention is required to get profiles defined. There is no default
profile that is accepted by CSI plugins.


Diffs
-

  include/mesos/module/volume_profile.hpp PRE-CREATION 
  src/Makefile.am 05e8b950a3ee13f7b2e8af9416495f2827138449 
  src/resource_provider/volume_profile.cpp PRE-CREATION 


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


Testing
---

See end of chain.


Thanks,

Joseph Wu

Re: Review Request 63971: Defined a module interface for translating volume profiles.

[Joseph Wu]

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

(Updated Dec. 5, 2017, 2:06 p.m.)


Review request for mesos, Chun-Hung Hsiao and Jie Yu.


Changes
---

Addressed comments.
Return value of `translate` was changed to only have a single VolumeCapability 
instead of an array of them.


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


Repository: mesos


Description
---

This module is currently intended for use by the Storage Local
Resource Provider (SLRP), but may be used by other components if
those components use any of the affected Container Storage Interface
(CSI) requests. The affected calls are listed in the module's comments.


Diffs (updated)
-

  include/mesos/mesos.proto 5b93478ed1a8250b05c44ec8b4d1f660a2053b87 
  include/mesos/resource_provider/volume_profile.hpp PRE-CREATION 
  include/mesos/v1/mesos.proto 22b3c6889d975147b001ef145ba4fc382debf70a 


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

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


Testing (updated)
---

See end of chain.


Thanks,

Joseph Wu

Review Request 64351: Moved generated CSI code into public directories.

[Joseph Wu]

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

Review request for mesos, Chun-Hung Hsiao and Jie Yu.


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


Repository: mesos


Description
---

This moves the Container Storage Interface (CSI) code generated from
its protobufs into the `include` directory so that other public
headers can use them.  The `csi/spec.hpp` was moved too, because
we prefer to include a layer of header indirection when adding
generated headers into non-generated headers.

An upcoming module specifically for CSI support will need to return
objects defined in the CSI specification.  And because modules have
public interfaces, we'll need the generated headers to be public too.


Diffs
-

  src/Makefile.am 05e8b950a3ee13f7b2e8af9416495f2827138449 
  src/csi/client.hpp 91dda0d44544c8901a9f71ef7f8338205b8ed598 
  src/csi/spec.hpp c819be30312e32275b2d17ade35ba9f500f81b80 
  src/tests/mock_csi_plugin.hpp 162dc6499a7ae880b30ec801ed399b7a3438bdc6 


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


Testing
---

See end of chain.


Thanks,

Joseph Wu

Re: Review Request 64010: Added `SlaveInfo` parameter to Allocator::updateSlave().

[Benno Evers]

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

(Updated Dec. 5, 2017, 9:26 p.m.)


Review request for mesos and Vinod Kone.


Changes
---

Rebased onto master.


Repository: mesos


Description
---

This is done mainly in preparation for the upcoming agent
reconfiguration patches, where it will become possible that
`updateSlave()` is passed a different SlaveInfo than `addSlave()`,
and schedulers who rely on some fields in it for their scheduling
decisions need to be able to check and compare them.

Additionally, the HierarchicalDRFAllocator was changed to store
the full SlaveInfo for every slave instead of storing domain and
hostname separately.


Diffs (updated)
-

  include/mesos/allocator/allocator.hpp 
acb9e4f6a843e64c915c43c218d8a533ca6b 
  src/master/allocator/mesos/allocator.hpp 
48254b6e0974bdc16ffda04d3d271538048d3206 
  src/master/allocator/mesos/hierarchical.hpp 
41ffe17def133d7e735afa26a6fedd154e51f4b4 
  src/master/allocator/mesos/hierarchical.cpp 
4bc9fb6f787224114f1285937cf979ace46d8ea3 
  src/master/master.cpp e8257e786b411a0f569559526918a6c49c63875a 
  src/tests/allocator.hpp fc5d9efc8dab3bd971fa8938e3f82e8291c4ab9d 
  src/tests/hierarchical_allocator_tests.cpp 
154049c47f2dae2df8d1bb4ddb5c48c478bb3d0e 


Diff: https://reviews.apache.org/r/64010/diff/9/

Changes: https://reviews.apache.org/r/64010/diff/8-9/


Testing
---

`make check`

https://jenkins.mesosphere.com/service/jenkins/view/Mesos/job/mesos/job/Mesos_CI-build/2299/


Thanks,

Benno Evers

Re: Review Request 64011: Updated master behaviour to update agent state on reregistration.

[Benno Evers]

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

(Updated Dec. 5, 2017, 9:25 p.m.)


Review request for mesos and Vinod Kone.


Changes
---

Rebased onto master.


Repository: mesos


Description
---

When an agent reregisters, the master will now always update
the agent information it holds in memory, and will write any
changes back to the registry if necessary.

Note that most tests for this are added in a later review, since they
require the capability to actually restart the agent with
a changed state to effectively test the masters response to that.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.hpp 
41ffe17def133d7e735afa26a6fedd154e51f4b4 
  src/master/allocator/mesos/hierarchical.cpp 
4bc9fb6f787224114f1285937cf979ace46d8ea3 
  src/master/master.hpp a721131e8f37e8d872da2b592124d72a19ec1763 
  src/master/master.cpp e8257e786b411a0f569559526918a6c49c63875a 
  src/tests/hierarchical_allocator_tests.cpp 
154049c47f2dae2df8d1bb4ddb5c48c478bb3d0e 


Diff: https://reviews.apache.org/r/64011/diff/10/

Changes: https://reviews.apache.org/r/64011/diff/9-10/


Testing
---


Thanks,

Benno Evers

Re: Review Request 64012: Added new `--reconfiguration_policy` slave flag and implementation.

[Vinod Kone]

> On Dec. 5, 2017, 8:18 p.m., Vinod Kone wrote:
> > src/slave/compatibility.hpp
> > Lines 33-36 (original), 32-41 (patched)
> > 
> >
> > Instead of specifying each field here, since it can go out of date, why 
> > not just say every field in `SlaveInfo` because that's what you are 
> > essentially doing?

I'll fix this while committing.


- Vinod


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


On Dec. 5, 2017, 4:02 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64012/
> ---
> 
> (Updated Dec. 5, 2017, 4:02 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This flag allows operators to select a set of permitted
> configuration changes that the slave will tolerate during
> recovery while still recovering running tasks and keeping
> the same agent id.
> 
> The previous behaviour of Mesos is reproduced exactly by setting
> this flag to "equal". For now only one additional policy is
> provided, "additive".
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 592489df070a0c4fdee814a4a7b613e62a544f88 
>   src/Makefile.am 08d29ab58fe5c7f5e0c9173b504787ec254ed99b 
>   src/slave/compatibility.hpp PRE-CREATION 
>   src/slave/compatibility.cpp PRE-CREATION 
>   src/slave/flags.hpp f25d8aff29d0698eaf06dd40a86a35e43bc9e094 
>   src/slave/flags.cpp 34edf4cb4a1afc366e6b6b4ddf78e90e49853fdf 
>   src/slave/slave.hpp 643d8559244f0842e82b293c1ef99cb26e111059 
>   src/slave/slave.cpp c07e25f668bef6b6fa3cae4b69ba90de3eb3bdcc 
>   src/tests/CMakeLists.txt 5e5202079c9798b91028e845490b747d4d1d0a66 
>   src/tests/master_tests.cpp 08742ec878b29ef275fb40e857efffcf4c2f9ac2 
>   src/tests/slave_compatibility_tests.cpp PRE-CREATION 
>   src/tests/slave_recovery_tests.cpp 7674e607ee5bb7efbceb81c3e7291bc558d69935 
>   src/tests/slave_tests.cpp a7f6658fa467914cd74691b5631d26d85ba7c1eb 
> 
> 
> Diff: https://reviews.apache.org/r/64012/diff/7/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 64101: Skip registry update when nothing changed.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Dec. 5, 2017, 1:36 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64101/
> ---
> 
> (Updated Dec. 5, 2017, 1:36 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Most of the time, agents will not change their configuration
> when they restart, so we can skip updating the registry in this
> case.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 883793a226849434eb833c3027d121635a86fdc4 
> 
> 
> Diff: https://reviews.apache.org/r/64101/diff/4/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> https://jenkins.mesosphere.com/service/jenkins/view/Mesos/job/mesos/job/Mesos_CI-build/2302/
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 64169: Check if slave is marked gone during reregistration.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Dec. 4, 2017, 11:43 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64169/
> ---
> 
> (Updated Dec. 4, 2017, 11:43 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit adds checks that test whether a
> re-registering agent is being marked gone or has been
> marked gone during the re-registration attempt.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 883793a226849434eb833c3027d121635a86fdc4 
> 
> 
> Diff: https://reviews.apache.org/r/64169/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 64012: Added new `--reconfiguration_policy` slave flag and implementation.

[Vinod Kone]

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


Fix it, then Ship it!





src/slave/compatibility.hpp
Lines 33-36 (original), 32-41 (patched)


Instead of specifying each field here, since it can go out of date, why not 
just say every field in `SlaveInfo` because that's what you are essentially 
doing?


- Vinod Kone


On Dec. 5, 2017, 4:02 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64012/
> ---
> 
> (Updated Dec. 5, 2017, 4:02 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This flag allows operators to select a set of permitted
> configuration changes that the slave will tolerate during
> recovery while still recovering running tasks and keeping
> the same agent id.
> 
> The previous behaviour of Mesos is reproduced exactly by setting
> this flag to "equal". For now only one additional policy is
> provided, "additive".
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 592489df070a0c4fdee814a4a7b613e62a544f88 
>   src/Makefile.am 08d29ab58fe5c7f5e0c9173b504787ec254ed99b 
>   src/slave/compatibility.hpp PRE-CREATION 
>   src/slave/compatibility.cpp PRE-CREATION 
>   src/slave/flags.hpp f25d8aff29d0698eaf06dd40a86a35e43bc9e094 
>   src/slave/flags.cpp 34edf4cb4a1afc366e6b6b4ddf78e90e49853fdf 
>   src/slave/slave.hpp 643d8559244f0842e82b293c1ef99cb26e111059 
>   src/slave/slave.cpp c07e25f668bef6b6fa3cae4b69ba90de3eb3bdcc 
>   src/tests/CMakeLists.txt 5e5202079c9798b91028e845490b747d4d1d0a66 
>   src/tests/master_tests.cpp 08742ec878b29ef275fb40e857efffcf4c2f9ac2 
>   src/tests/slave_compatibility_tests.cpp PRE-CREATION 
>   src/tests/slave_recovery_tests.cpp 7674e607ee5bb7efbceb81c3e7291bc558d69935 
>   src/tests/slave_tests.cpp a7f6658fa467914cd74691b5631d26d85ba7c1eb 
> 
> 
> Diff: https://reviews.apache.org/r/64012/diff/7/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 64342: Correctly updated total resources when agent capabilities changed.

[Alexander Rukletsov]

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


Ship it!




Ship It!

- Alexander Rukletsov


On Dec. 5, 2017, 3:33 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64342/
> ---
> 
> (Updated Dec. 5, 2017, 3:33 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Bannier.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Due to the short-circuiting behaviour of C++ boolean operations,
> an agent that re-registers with changed agent capabilities would
> not have its total updated.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 715650ee9cb15aed1d1e58badf70fc09e26d13c1 
> 
> 
> Diff: https://reviews.apache.org/r/64342/diff/1/
> 
> 
> Testing
> ---
> 
> https://jenkins.mesosphere.com/service/jenkins/view/Mesos/job/mesos/job/Mesos_CI-build/2314/
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 63641: Used move for events consumption in master.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Nov. 7, 2017, 9:42 a.m., Dmitry Zhuk wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63641/
> ---
> 
> (Updated Nov. 7, 2017, 9:42 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Michael Park.
> 
> 
> Bugs: MESOS-6972
> https://issues.apache.org/jira/browse/MESOS-6972
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used move for events consumption in master.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 448be3b4a3eb3d0cbd34a39b19640d15cf3fc2af 
>   src/master/master.cpp 31991b5f7f7f4e7dc73d71a3160348bfc0a627c3 
> 
> 
> Diff: https://reviews.apache.org/r/63641/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Dmitry Zhuk
> 
>

Re: Review Request 63638: Added callable once support in Future.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Nov. 21, 2017, 9:51 a.m., Dmitry Zhuk wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63638/
> ---
> 
> (Updated Nov. 21, 2017, 9:51 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Michael Park.
> 
> 
> Bugs: MESOS-6972
> https://issues.apache.org/jira/browse/MESOS-6972
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `Future` guarantees that callbacks are called at most once, so it can
> use `lambda::CallableOnce` to expicitly declare this, and allow
> corresponding optimizations with moves.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/future.hpp 
> d48dfd795fdc4021270621522a365e3cad4102a6 
>   3rdparty/libprocess/src/tests/future_tests.cpp 
> bb7a1c83e136136088e60e4d187e2a52df49f454 
> 
> 
> Diff: https://reviews.apache.org/r/63638/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Dmitry Zhuk
> 
>

Re: Review Request 63637: Added callable once support in defer.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Dec. 5, 2017, 4:20 a.m., Dmitry Zhuk wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63637/
> ---
> 
> (Updated Dec. 5, 2017, 4:20 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Michael Park.
> 
> 
> Bugs: MESOS-6972
> https://issues.apache.org/jira/browse/MESOS-6972
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This allows `defer` result to be converted to `CallableOnce`, ensuring
> that bound arguments are moved, when call is made, and avoiding making
> copies of bound arguments.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/deferred.hpp 
> 950f1cccaf93298664a95ba1fd2c5d2a42deb725 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> d1b30b0754c55fd765885ddcfe9f9f68ba509ac1 
> 
> 
> Diff: https://reviews.apache.org/r/63637/diff/4/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Dmitry Zhuk
> 
>

Re: Review Request 64011: Updated master behaviour to update agent state on reregistration.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Dec. 5, 2017, 7:03 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64011/
> ---
> 
> (Updated Dec. 5, 2017, 7:03 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When an agent reregisters, the master will now always update
> the agent information it holds in memory, and will write any
> changes back to the registry if necessary.
> 
> Note that most tests for this are added in a later review, since they
> require the capability to actually restart the agent with
> a changed state to effectively test the masters response to that.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 41ffe17def133d7e735afa26a6fedd154e51f4b4 
>   src/master/allocator/mesos/hierarchical.cpp 
> 4428541e63561f7c3bc3bc61f4b47e35216f6442 
>   src/master/master.hpp 448be3b4a3eb3d0cbd34a39b19640d15cf3fc2af 
>   src/master/master.cpp 31991b5f7f7f4e7dc73d71a3160348bfc0a627c3 
>   src/tests/hierarchical_allocator_tests.cpp 
> 154049c47f2dae2df8d1bb4ddb5c48c478bb3d0e 
> 
> 
> Diff: https://reviews.apache.org/r/64011/diff/9/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 64338: Reduced number of supported arguments in _Deferred conversion operators.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Dec. 5, 2017, 4:18 a.m., Dmitry Zhuk wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64338/
> ---
> 
> (Updated Dec. 5, 2017, 4:18 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Conversion of `_Deferred` to `std::function` and `Deferred` currently
> supports up to 12 parameters in function signature. However, this is
> unnecessary and is a huge overhead. Most usages require just one
> parameter (e.g. when `defer` is used with `Future`). And there are few
> usages with two parameters (in `master.cpp` to initialize allocator, and
> in `slave.cpp` to install signal handler).
> This number of parameters is different from the number of parameters
> passed to `defer`, but it's related and can be defined as maximum number
> of placeholders that can be passed to `defer`.
> 
> Given that `deferred.hpp` is indirectly included in most source files,
> it is beneficial to keep this number low. This patch changes maximum
> number of parameters to 2.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/deferred.hpp 
> 950f1cccaf93298664a95ba1fd2c5d2a42deb725 
> 
> 
> Diff: https://reviews.apache.org/r/64338/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Dmitry Zhuk
> 
>

Re: Review Request 64011: Updated master behaviour to update agent state on reregistration.

[Benno Evers]

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

(Updated Dec. 5, 2017, 7:03 p.m.)


Review request for mesos and Vinod Kone.


Changes
---

Update test to pause the clock while running and rebase onto latest master.


Repository: mesos


Description
---

When an agent reregisters, the master will now always update
the agent information it holds in memory, and will write any
changes back to the registry if necessary.

Note that most tests for this are added in a later review, since they
require the capability to actually restart the agent with
a changed state to effectively test the masters response to that.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.hpp 
41ffe17def133d7e735afa26a6fedd154e51f4b4 
  src/master/allocator/mesos/hierarchical.cpp 
4428541e63561f7c3bc3bc61f4b47e35216f6442 
  src/master/master.hpp 448be3b4a3eb3d0cbd34a39b19640d15cf3fc2af 
  src/master/master.cpp 31991b5f7f7f4e7dc73d71a3160348bfc0a627c3 
  src/tests/hierarchical_allocator_tests.cpp 
154049c47f2dae2df8d1bb4ddb5c48c478bb3d0e 


Diff: https://reviews.apache.org/r/64011/diff/9/

Changes: https://reviews.apache.org/r/64011/diff/8-9/


Testing
---


Thanks,

Benno Evers

Re: Review Request 63635: Prepared defer for use in callable once contexts.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Nov. 7, 2017, 8:51 a.m., Dmitry Zhuk wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63635/
> ---
> 
> (Updated Nov. 7, 2017, 8:51 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Michael Park.
> 
> 
> Bugs: MESOS-6972
> https://issues.apache.org/jira/browse/MESOS-6972
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This changes `defer` to use `lambda::partial` instead of `std::bind`,
> which allows it be used in callbale once contexts.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/defer.hpp 
> 1c114a8d8e3a90fed9be0519ac093e402ebcd977 
>   3rdparty/libprocess/include/process/deferred.hpp 
> 950f1cccaf93298664a95ba1fd2c5d2a42deb725 
> 
> 
> Diff: https://reviews.apache.org/r/63635/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Dmitry Zhuk
> 
>

Re: Review Request 64008: Activated AGENT_UPDATE master capability.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Dec. 1, 2017, 6:32 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64008/
> ---
> 
> (Updated Dec. 1, 2017, 6:32 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Activated AGENT_UPDATE master capability.
> 
> 
> Diffs
> -
> 
>   src/master/constants.cpp 55eecfbe5e48515767eadece54c8d0488dfaa830 
>   src/tests/master_tests.cpp 08742ec878b29ef275fb40e857efffcf4c2f9ac2 
> 
> 
> Diff: https://reviews.apache.org/r/64008/diff/7/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 64011: Updated master behaviour to update agent state on reregistration.

[Vinod Kone]

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


Fix it, then Ship it!





src/tests/hierarchical_allocator_tests.cpp
Line 6283 (original)


Let's actually pause the clock in this test.


- Vinod Kone


On Dec. 5, 2017, 4:14 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64011/
> ---
> 
> (Updated Dec. 5, 2017, 4:14 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When an agent reregisters, the master will now always update
> the agent information it holds in memory, and will write any
> changes back to the registry if necessary.
> 
> Note that most tests for this are added in a later review, since they
> require the capability to actually restart the agent with
> a changed state to effectively test the masters response to that.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 3c87dc797cf70f3aa48b1ed9f86d673d4ea2fe76 
>   src/master/allocator/mesos/hierarchical.cpp 
> 715650ee9cb15aed1d1e58badf70fc09e26d13c1 
>   src/master/master.hpp 5d2ae658070d9c5a0bc630c15ff89dc449857f46 
>   src/master/master.cpp 883793a226849434eb833c3027d121635a86fdc4 
>   src/tests/hierarchical_allocator_tests.cpp 
> 0309074bab180be122c9b0074981e6f69c97feee 
> 
> 
> Diff: https://reviews.apache.org/r/64011/diff/8/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 64010: Added `SlaveInfo` parameter to Allocator::updateSlave().

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Dec. 5, 2017, 5:39 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64010/
> ---
> 
> (Updated Dec. 5, 2017, 5:39 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is done mainly in preparation for the upcoming agent
> reconfiguration patches, where it will become possible that
> `updateSlave()` is passed a different SlaveInfo than `addSlave()`,
> and schedulers who rely on some fields in it for their scheduling
> decisions need to be able to check and compare them.
> 
> Additionally, the HierarchicalDRFAllocator was changed to store
> the full SlaveInfo for every slave instead of storing domain and
> hostname separately.
> 
> 
> Diffs
> -
> 
>   include/mesos/allocator/allocator.hpp 
> acb9e4f6a843e64c915c43c218d8a533ca6b 
>   src/master/allocator/mesos/allocator.hpp 
> 48254b6e0974bdc16ffda04d3d271538048d3206 
>   src/master/allocator/mesos/hierarchical.hpp 
> 41ffe17def133d7e735afa26a6fedd154e51f4b4 
>   src/master/allocator/mesos/hierarchical.cpp 
> 4428541e63561f7c3bc3bc61f4b47e35216f6442 
>   src/master/master.cpp 31991b5f7f7f4e7dc73d71a3160348bfc0a627c3 
>   src/tests/allocator.hpp fc5d9efc8dab3bd971fa8938e3f82e8291c4ab9d 
>   src/tests/hierarchical_allocator_tests.cpp 
> 154049c47f2dae2df8d1bb4ddb5c48c478bb3d0e 
> 
> 
> Diff: https://reviews.apache.org/r/64010/diff/8/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> https://jenkins.mesosphere.com/service/jenkins/view/Mesos/job/mesos/job/Mesos_CI-build/2299/
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 64009: Added new UpdateSlave registry operation.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Dec. 4, 2017, 3:28 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64009/
> ---
> 
> (Updated Dec. 4, 2017, 3:28 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Michael Park, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This operation can be used to update the stored state
> of an existing, admitted slave.
> 
> 
> Diffs
> -
> 
>   src/master/registry_operations.hpp 5b78306fb2e2bd7e30200f112f0381a595c7b25d 
>   src/master/registry_operations.cpp 1e1eadb539482113e8ca8cb0542e5aaf34a38b41 
>   src/tests/registrar_tests.cpp 21d78e90e57c145aa9f4b26f1a2f0278390b98d4 
> 
> 
> Diff: https://reviews.apache.org/r/64009/diff/6/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 64252: Convert resource format of messages entering master.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Dec. 4, 2017, 3:27 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64252/
> ---
> 
> (Updated Dec. 4, 2017, 3:27 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Convert resource format of messages entering master.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 883793a226849434eb833c3027d121635a86fdc4 
>   src/master/registry_operations.cpp 1e1eadb539482113e8ca8cb0542e5aaf34a38b41 
> 
> 
> Diff: https://reviews.apache.org/r/64252/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 64146: Added offer operation update acknowledgement to the agent.

[Jie Yu]

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


Fix it, then Ship it!





src/slave/slave.cpp
Lines 3860 (patched)


`operation != nullptr`


- Jie Yu


On Dec. 5, 2017, 7:07 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64146/
> ---
> 
> (Updated Dec. 5, 2017, 7:07 a.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Jie Yu.
> 
> 
> Bugs: MESOS-8186
> https://issues.apache.org/jira/browse/MESOS-8186
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The agent's offerOperationUpdateAcknowlegement handler is
> updated to pass acknowledgements to the resource provider
> manager.
> 
> The agent's resource provider message handler is also
> updated to avoid removing offer operations, since this
> should actually be done upon acknowledgement of the update.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp c07e25f668bef6b6fa3cae4b69ba90de3eb3bdcc 
> 
> 
> Diff: https://reviews.apache.org/r/64146/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 64347: Made libprocess event movable only.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Dec. 5, 2017, 10:06 a.m., Dmitry Zhuk wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64347/
> ---
> 
> (Updated Dec. 5, 2017, 10:06 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made libprocess events movable only.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/event.hpp 
> 571ffb1ea5c4787f10c2f4dee249aadc8b904bc6 
> 
> 
> Diff: https://reviews.apache.org/r/64347/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Dmitry Zhuk
> 
>

Re: Review Request 64143: Added ACKNOWLEDGE event to the resource provider API.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Dec. 5, 2017, 7:33 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64143/
> ---
> 
> (Updated Dec. 5, 2017, 7:33 a.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Jie Yu.
> 
> 
> Bugs: MESOS-8186
> https://issues.apache.org/jira/browse/MESOS-8186
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The new event is used to send offer operation status udpate
> acknowledgements to resource providers.
> 
> 
> Diffs
> -
> 
>   include/mesos/resource_provider/resource_provider.proto 
> 2ce71f4c3e97e89c226966c347c2817e2bab9c1b 
>   include/mesos/v1/resource_provider/resource_provider.proto 
> 465b11dafd7c2e3b9476696ed75c1c077d6c8eeb 
>   src/resource_provider/storage/provider.cpp 
> d35b0d02992e3730ca47906b34c21e1ba9c653e7 
>   src/tests/mesos.hpp 3a9b1fbd4301c6cccbc770329ff71ed8ff7e86a2 
> 
> 
> Diff: https://reviews.apache.org/r/64143/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 64144: Made master acknowledge offer operation updates when 'id' isn't set.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Dec. 5, 2017, 7:06 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64144/
> ---
> 
> (Updated Dec. 5, 2017, 7:06 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gaston Kleiman, Jie Yu, and Jan 
> Schlicht.
> 
> 
> Bugs: MESOS-8193
> https://issues.apache.org/jira/browse/MESOS-8193
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When a framework does not request feedback about an operation,
> the master should acknowledge offer operation status updates
> to the agent so that the updates are not retried.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 883793a226849434eb833c3027d121635a86fdc4 
> 
> 
> Diff: https://reviews.apache.org/r/64144/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 63761: Replaced std::shared_ptr with std::unique_ptr in dispatch.

[Dmitry Zhuk]

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

(Updated Dec. 5, 2017, 6:07 p.m.)


Review request for mesos, Benjamin Mahler and Michael Park.


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


Repository: mesos


Description
---

Since `dispatch` can now handle movable only parameters, `Promise` and
functor can be safely wrapped into `std::unique_ptr` for efficiency.


Diffs (updated)
-

  3rdparty/libprocess/include/process/dispatch.hpp 
155f362a888caef5ce2db803a109400ca4c7ec37 
  3rdparty/libprocess/include/process/event.hpp 
571ffb1ea5c4787f10c2f4dee249aadc8b904bc6 
  3rdparty/libprocess/src/process.cpp 64bcce215d19558dd493e30e96ca16577fe0722a 


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

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


Testing
---

make check


Thanks,

Dmitry Zhuk

Review Request 64347: Made libprocess event movable only.

[Dmitry Zhuk]

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

Review request for mesos, Benjamin Mahler and Michael Park.


Repository: mesos


Description
---

Made libprocess events movable only.


Diffs
-

  3rdparty/libprocess/include/process/event.hpp 
571ffb1ea5c4787f10c2f4dee249aadc8b904bc6 


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


Testing
---


Thanks,

Dmitry Zhuk

Re: Review Request 63631: Separated event visiting and consumption.

[Dmitry Zhuk]

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

(Updated Dec. 5, 2017, 6 p.m.)


Review request for mesos, Benjamin Mahler and Michael Park.


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


Repository: mesos


Description
---

This introduces `EventConsumer` interface, which add support of events
with movable only data. This allows consumers to move data out of
event, rather than copying it. This is required to implement movable
only objects support in `defer` to guarantee that deferred functor is
invoked only once, allowing deferred parameters to be moved into call.


Diffs (updated)
-

  3rdparty/libprocess/include/process/event.hpp 
571ffb1ea5c4787f10c2f4dee249aadc8b904bc6 
  3rdparty/libprocess/include/process/process.hpp 
d88c2f241a04cac3ec2e1d9fd4b323643ad24ae4 
  3rdparty/libprocess/include/process/protobuf.hpp 
08605db4aa7ae782f1e5a1f692db9d46f899f842 
  3rdparty/libprocess/include/process/run.hpp 
46a182b2dd4ab7add6d6f547e83958b19fa2a0f2 
  3rdparty/libprocess/src/process.cpp 64bcce215d19558dd493e30e96ca16577fe0722a 
  3rdparty/libprocess/src/tests/benchmarks.cpp 
2c6de626c3fdf5ebd3a7aa5793553215c42dd494 
  3rdparty/libprocess/src/tests/process_tests.cpp 
d1b30b0754c55fd765885ddcfe9f9f68ba509ac1 


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

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


Testing
---


Thanks,

Dmitry Zhuk

Re: Review Request 64010: Added `SlaveInfo` parameter to Allocator::updateSlave().

[Benno Evers]

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

(Updated Dec. 5, 2017, 5:39 p.m.)


Review request for mesos and Vinod Kone.


Changes
---

Rebased onto latest master.


Repository: mesos


Description
---

This is done mainly in preparation for the upcoming agent
reconfiguration patches, where it will become possible that
`updateSlave()` is passed a different SlaveInfo than `addSlave()`,
and schedulers who rely on some fields in it for their scheduling
decisions need to be able to check and compare them.

Additionally, the HierarchicalDRFAllocator was changed to store
the full SlaveInfo for every slave instead of storing domain and
hostname separately.


Diffs (updated)
-

  include/mesos/allocator/allocator.hpp 
acb9e4f6a843e64c915c43c218d8a533ca6b 
  src/master/allocator/mesos/allocator.hpp 
48254b6e0974bdc16ffda04d3d271538048d3206 
  src/master/allocator/mesos/hierarchical.hpp 
41ffe17def133d7e735afa26a6fedd154e51f4b4 
  src/master/allocator/mesos/hierarchical.cpp 
4428541e63561f7c3bc3bc61f4b47e35216f6442 
  src/master/master.cpp 31991b5f7f7f4e7dc73d71a3160348bfc0a627c3 
  src/tests/allocator.hpp fc5d9efc8dab3bd971fa8938e3f82e8291c4ab9d 
  src/tests/hierarchical_allocator_tests.cpp 
154049c47f2dae2df8d1bb4ddb5c48c478bb3d0e 


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

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


Testing
---

`make check`

https://jenkins.mesosphere.com/service/jenkins/view/Mesos/job/mesos/job/Mesos_CI-build/2299/


Thanks,

Benno Evers

Re: Review Request 64332: Removed `constexpr`-ness from `cpp17::invoke`.

[Benjamin Bannier]

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


Ship it!




- Benjamin Bannier


On Dec. 5, 2017, 9:55 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64332/
> ---
> 
> (Updated Dec. 5, 2017, 9:55 a.m.)
> 
> 
> Review request for mesos and Benjamin Bannier.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `std::invoke` is not marked `constexpr`.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/cpp17.hpp 
> 3c4cc5345c94270785fa1594d7310af2b4dcf402 
> 
> 
> Diff: https://reviews.apache.org/r/64332/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 64331: Added more `cpp17::invoke` test cases.

[Benjamin Bannier]

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


Ship it!




Ship It!

- Benjamin Bannier


On Dec. 5, 2017, 9:31 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64331/
> ---
> 
> (Updated Dec. 5, 2017, 9:31 a.m.)
> 
> 
> Review request for mesos and Benjamin Bannier.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/tests/cpp17_tests.cpp 
> 37d817f1af90cc6a17c5d7f17cf0571fd3f4ac7c 
> 
> 
> Diff: https://reviews.apache.org/r/64331/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 64151: Added a V1 API call to list resource providers.

[Benjamin Bannier]

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




include/mesos/agent/agent.proto
Lines 60 (patched)


Let's add some minimal documentation here.



include/mesos/v1/agent/agent.proto
Lines 60 (patched)


Let's add some minimal documentation here.



src/resource_provider/manager.hpp
Lines 28 (patched)


Unused?



src/resource_provider/manager.hpp
Lines 63 (patched)


Could we explicitly include `mesos.hpp` for `ResourceProviderInfo`?



src/resource_provider/manager.cpp
Lines 466 (patched)


`foreachvalue`? We should also include `stout/foreach.hpp` here.



src/slave/http.cpp
Lines 1862 (patched)


Let's create a ticket tracking authorization of this call and mention it in 
a `TODO` here.



src/slave/http.cpp
Lines 1879-1881 (patched)


This could become slightly less nasty if we would declare outside the loop a

agent::Response::GetResourceProviders* getResourceProviders =
  response.mutable_get_resource_providers();
  
It does not help a lot though.



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


This would be much simpler with a `MockResourceProvider`.



src/tests/api_tests.cpp
Lines 6094-6095 (patched)


EXPECT_EQ(
  1, v1Response->get_resource_providers().resource_providers_size());



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


We could bind to a const ref here.



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


This needs to be tweak if we use a `MockResourceProvider`.



src/tests/resource_provider_manager_tests.cpp
Lines 839 (patched)


Could we use a `MockResourceProvider` here as well?



src/tests/resource_provider_manager_tests.cpp
Lines 880 (patched)


This needs to be an assert.



src/tests/resource_provider_manager_tests.cpp
Lines 881 (patched)


This needs to be tweaked if we use a `MockResourceProvider`.


- Benjamin Bannier


On Dec. 5, 2017, 3:34 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64151/
> ---
> 
> (Updated Dec. 5, 2017, 3:34 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Bugs: MESOS-8270
> https://issues.apache.org/jira/browse/MESOS-8270
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The 'GET_RESOURCE_PROVIDERS' call will list all subscribed local
> resource providers of an agent.
> 
> 
> Diffs
> -
> 
>   include/mesos/agent/agent.proto 0f92f73ba0f7729f0ba7cd89a692ab3685125e8b 
>   include/mesos/v1/agent/agent.proto 012ffef5f0dd7720fa95ae484c99479aaf256d7b 
>   src/resource_provider/manager.hpp c2aeb15b0b8ebd167ddf9c42c9fc396d4c0a126b 
>   src/resource_provider/manager.cpp 8d8b2f1396230d6edba590b8b7e9b4ca51366efe 
>   src/slave/http.hpp a51831cdcebc1998ce6d4c3c19285e598a4ec9a3 
>   src/slave/http.cpp fd0e809d5dc7722e573b66621f75d791f0911dfa 
>   src/slave/validation.cpp 32781fd8f124f71e61744804aec3fe4da59a5df2 
>   src/tests/api_tests.cpp 53c705ed4775846ba7eb42da57f962895f5669aa 
>   src/tests/resource_provider_manager_tests.cpp 
> a4c19ca769e66110d9aba0bae4792df9db3fed01 
> 
> 
> Diff: https://reviews.apache.org/r/64151/diff/6/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 63631: Separated event visiting and consumption.

[Michael Park]

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


Fix it, then Ship it!





3rdparty/libprocess/include/process/event.hpp
Lines 91-127 (original), 103-144 (patched)


I noticed that we have 2 types of messages:
  (1) copyable but not assignable
  (2) not copyable nor assignable

I think we should make all of the message properly move-only. (e.g., the 
changes we made to `HttpEvent`).
For most of them we just need to remove the `const` on the members.



3rdparty/libprocess/include/process/event.hpp
Lines 114-117 (original), 126-129 (patched)


This comment needs to be updated.



3rdparty/libprocess/include/process/event.hpp
Lines 155-158 (original), 178-181 (patched)


Let's clean these up and pull them up and declare them `= delete;`.
Here and below.


- Michael Park


On Dec. 4, 2017, 8:01 a.m., Dmitry Zhuk wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63631/
> ---
> 
> (Updated Dec. 4, 2017, 8:01 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Michael Park.
> 
> 
> Bugs: MESOS-6972
> https://issues.apache.org/jira/browse/MESOS-6972
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This introduces `EventConsumer` interface, which add support of events
> with movable only data. This allows consumers to move data out of
> event, rather than copying it. This is required to implement movable
> only objects support in `defer` to guarantee that deferred functor is
> invoked only once, allowing deferred parameters to be moved into call.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/event.hpp 
> 571ffb1ea5c4787f10c2f4dee249aadc8b904bc6 
>   3rdparty/libprocess/include/process/process.hpp 
> d88c2f241a04cac3ec2e1d9fd4b323643ad24ae4 
>   3rdparty/libprocess/include/process/protobuf.hpp 
> 08605db4aa7ae782f1e5a1f692db9d46f899f842 
>   3rdparty/libprocess/include/process/run.hpp 
> 46a182b2dd4ab7add6d6f547e83958b19fa2a0f2 
>   3rdparty/libprocess/src/process.cpp 
> 64bcce215d19558dd493e30e96ca16577fe0722a 
>   3rdparty/libprocess/src/tests/benchmarks.cpp 
> 2c6de626c3fdf5ebd3a7aa5793553215c42dd494 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> d1b30b0754c55fd765885ddcfe9f9f68ba509ac1 
> 
> 
> Diff: https://reviews.apache.org/r/63631/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Dmitry Zhuk
> 
>

Re: Review Request 64342: Correctly updated total resources when agent capabilities changed.

[Benjamin Bannier]

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


Ship it!




Ship It!

- Benjamin Bannier


On Dec. 5, 2017, 4:33 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64342/
> ---
> 
> (Updated Dec. 5, 2017, 4:33 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Bannier.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Due to the short-circuiting behaviour of C++ boolean operations,
> an agent that re-registers with changed agent capabilities would
> not have its total updated.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 715650ee9cb15aed1d1e58badf70fc09e26d13c1 
> 
> 
> Diff: https://reviews.apache.org/r/64342/diff/1/
> 
> 
> Testing
> ---
> 
> https://jenkins.mesosphere.com/service/jenkins/view/Mesos/job/mesos/job/Mesos_CI-build/2314/
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 64011: Updated master behaviour to update agent state on reregistration.

[Benno Evers]

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

(Updated Dec. 5, 2017, 4:14 p.m.)


Review request for mesos and Vinod Kone.


Repository: mesos


Description
---

When an agent reregisters, the master will now always update
the agent information it holds in memory, and will write any
changes back to the registry if necessary.

Note that most tests for this are added in a later review, since they
require the capability to actually restart the agent with
a changed state to effectively test the masters response to that.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.hpp 
3c87dc797cf70f3aa48b1ed9f86d673d4ea2fe76 
  src/master/allocator/mesos/hierarchical.cpp 
715650ee9cb15aed1d1e58badf70fc09e26d13c1 
  src/master/master.hpp 5d2ae658070d9c5a0bc630c15ff89dc449857f46 
  src/master/master.cpp 883793a226849434eb833c3027d121635a86fdc4 
  src/tests/hierarchical_allocator_tests.cpp 
0309074bab180be122c9b0074981e6f69c97feee 


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

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


Testing
---


Thanks,

Benno Evers

Re: Review Request 64012: Added new `--reconfiguration_policy` slave flag and implementation.

[Benno Evers]

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

(Updated Dec. 5, 2017, 4:02 p.m.)


Review request for mesos and Vinod Kone.


Summary (updated)
-

Added new `--reconfiguration_policy` slave flag and implementation.


Repository: mesos


Description (updated)
---

This flag allows operators to select a set of permitted
configuration changes that the slave will tolerate during
recovery while still recovering running tasks and keeping
the same agent id.

The previous behaviour of Mesos is reproduced exactly by setting
this flag to "equal". For now only one additional policy is
provided, "additive".


Diffs (updated)
-

  src/CMakeLists.txt 592489df070a0c4fdee814a4a7b613e62a544f88 
  src/Makefile.am 08d29ab58fe5c7f5e0c9173b504787ec254ed99b 
  src/slave/compatibility.hpp PRE-CREATION 
  src/slave/compatibility.cpp PRE-CREATION 
  src/slave/flags.hpp f25d8aff29d0698eaf06dd40a86a35e43bc9e094 
  src/slave/flags.cpp 34edf4cb4a1afc366e6b6b4ddf78e90e49853fdf 
  src/slave/slave.hpp 643d8559244f0842e82b293c1ef99cb26e111059 
  src/slave/slave.cpp c07e25f668bef6b6fa3cae4b69ba90de3eb3bdcc 
  src/tests/CMakeLists.txt 5e5202079c9798b91028e845490b747d4d1d0a66 
  src/tests/master_tests.cpp 08742ec878b29ef275fb40e857efffcf4c2f9ac2 
  src/tests/slave_compatibility_tests.cpp PRE-CREATION 
  src/tests/slave_recovery_tests.cpp 7674e607ee5bb7efbceb81c3e7291bc558d69935 
  src/tests/slave_tests.cpp a7f6658fa467914cd74691b5631d26d85ba7c1eb 


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

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


Testing
---


Thanks,

Benno Evers

Re: Review Request 64012: Added new --reconfiguration_compatibility slave flag and implementation.

[Benno Evers]

> On Dec. 4, 2017, 11:14 p.m., Vinod Kone wrote:
> > src/tests/slave_tests.cpp
> > Lines 8912 (patched)
> > 
> >
> > what about testing the resources?

I originally left that out because it has to assume quite a lot about the 
internal resource handling of master and slave, which didn't seem very relevant 
to agent reconfiguration and I thought it would be highly unlikely that a bug 
would be able to fail this and pass the test in `master_tests.cpp` where we 
actually check the resources. Of course, it turned out that there actually was 
a latent bug in the allocator which happens only in precisely the situation we 
have here: https://reviews.apache.org/r/64342/


- Benno


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


On Dec. 4, 2017, 4:33 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64012/
> ---
> 
> (Updated Dec. 4, 2017, 4:33 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This flag allows operators to weaken the checks performed by the agent
> when recovering state, in particular it allows to recover running tasks
> even when parts of the recovered SlaveInfo don't match the current
> state.
> 
> The set of possible changes is quite restricted for now,
> to avoid accidenetally introducing semantic bugs.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 592489df070a0c4fdee814a4a7b613e62a544f88 
>   src/Makefile.am 08d29ab58fe5c7f5e0c9173b504787ec254ed99b 
>   src/slave/compatibility.hpp PRE-CREATION 
>   src/slave/compatibility.cpp PRE-CREATION 
>   src/slave/flags.hpp f25d8aff29d0698eaf06dd40a86a35e43bc9e094 
>   src/slave/flags.cpp 34edf4cb4a1afc366e6b6b4ddf78e90e49853fdf 
>   src/slave/slave.hpp 643d8559244f0842e82b293c1ef99cb26e111059 
>   src/slave/slave.cpp c07e25f668bef6b6fa3cae4b69ba90de3eb3bdcc 
>   src/tests/CMakeLists.txt 5e5202079c9798b91028e845490b747d4d1d0a66 
>   src/tests/master_tests.cpp 08742ec878b29ef275fb40e857efffcf4c2f9ac2 
>   src/tests/slave_compatibility_tests.cpp PRE-CREATION 
>   src/tests/slave_recovery_tests.cpp 7674e607ee5bb7efbceb81c3e7291bc558d69935 
>   src/tests/slave_tests.cpp a7f6658fa467914cd74691b5631d26d85ba7c1eb 
> 
> 
> Diff: https://reviews.apache.org/r/64012/diff/6/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Review Request 64342: Correctly updated total resources when agent capabilities changed.

[Benno Evers]

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

Review request for mesos, Alexander Rukletsov and Benjamin Bannier.


Repository: mesos


Description
---

Due to the short-circuiting behaviour of C++ boolean operations,
an agent that re-registers with changed agent capabilities would
not have its total updated.


Diffs
-

  src/master/allocator/mesos/hierarchical.cpp 
715650ee9cb15aed1d1e58badf70fc09e26d13c1 


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


Testing
---

https://jenkins.mesosphere.com/service/jenkins/view/Mesos/job/mesos/job/Mesos_CI-build/2314/


Thanks,

Benno Evers

Re: Review Request 64337: Fixed CallableOnce operator() signature.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Dec. 5, 2017, 4:13 a.m., Dmitry Zhuk wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64337/
> ---
> 
> (Updated Dec. 5, 2017, 4:13 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This changes `operator()` signature to match the one defined in
> `CallableOnce` template parameter.
> Previously used form incorrectly specifies that `operator()` can be
> invoked with arbitrary number and types of parameters, which can break
> other templates using SFINAE to check if functor can be invoked with
> specific parameters.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/lambda.hpp 
> 96586bd572093d2874a617252ed22b9e6874236b 
> 
> 
> Diff: https://reviews.apache.org/r/64337/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Dmitry Zhuk
> 
>

Re: Review Request 63636: Added placeholder implementation.

[Dmitry Zhuk]

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

(Updated Dec. 5, 2017, 7:21 a.m.)


Review request for mesos, Benjamin Mahler and Michael Park.


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


Repository: mesos


Description
---

Added placeholder implementation.


Diffs
-

  3rdparty/stout/include/stout/lambda.hpp 
a61d97b69e7eebd057c94148d39c6e1fc3066017 


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


Testing
---


Thanks,

Dmitry Zhuk

Re: Review Request 63914: Changed agent reregistration to work with message directly.

[Dmitry Zhuk]

> On Dec. 4, 2017, 9:18 p.m., Benjamin Mahler wrote:
> > src/master/master.cpp
> > Lines 6492-6499 (patched)
> > 
> >
> > Hm.. I'm curious if you have any thoughts on avoiding copying it out of 
> > the message. We can look at that more closely in a different patch though :)

Simplest solution is to add
```
template 
std::vector convert(google::protobuf::RepeatedPtrField&& items)
```
and use it like this:
```
vector tasks = 
google::protobuf::convert(std::move(*message.mutable_tasks()));
```
but the problem is that it becomes difficult to track which parts of the 
message have been moved from. And I suppose `clang-tidy` will not be able to 
detect "use after move" in such case. So I haven't published this patch.
I was thinking about decomposing a message with moves and invoking continuation 
method with all individual message fields as parameters to make sure message 
goes out of scope, but I haven't tried this yet.

In many cases it's possible to avoid using `convert`, and use 
`RepeatedPtrField` instead. I did this for `tasks`, but the problem is still 
the same - difficult to track moved message parts.


- Dmitry


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


On Nov. 21, 2017, 5:53 p.m., Dmitry Zhuk wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63914/
> ---
> 
> (Updated Nov. 21, 2017, 5:53 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Michael Park.
> 
> 
> Bugs: MESOS-6972
> https://issues.apache.org/jira/browse/MESOS-6972
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `reregisterSlave` now accepts `ReregisterSlaveMessage&&`, which opts-out
> of using protobuf arena, and allows passing message through dispatch
> chain without making any copies.
> Conversion of repeated message fields to `std::vector`s is performed
> only when needed.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 0f8a2ac72c3484f911853c2994fc71a488d66d96 
>   src/master/master.cpp fadc78b2ca5d46b8cc12a794b428753aa79ac095 
>   src/master/validation.hpp ac54062ea09f97ad96bd17deb106ea89a57f394a 
>   src/master/validation.cpp 8b5848bfd8c069f34a92a9a68597955c6e0c2ee2 
>   src/tests/master_validation_tests.cpp 
> 0e1c8b490ebe10bc50b8b6b530fa85128b967584 
> 
> 
> Diff: https://reviews.apache.org/r/63914/diff/4/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Dmitry Zhuk
> 
>

Re: Review Request 63914: Changed agent reregistration to work with message directly.

[Dmitry Zhuk]

> On Dec. 4, 2017, 5:57 p.m., Michael Park wrote:
> > src/master/master.cpp
> > Line 6433 (original), 6394 (patched)
> > 
> >
> > I think these `message` to `message_` changes should either take the 
> > type of the mesage (e.g., `shutdown`), or simply a `s/message/outgoing/`. 
> > Having unrelated `message` and `message_` in the same scope will likely 
> > cause confusion.

Maybe it's better change `ReregisterSlaveMessage` name to `request` and keep 
`message` in other places?
I tried using names like `checkpointResourcesMessage` as bmahler suggested 
below, but lines became too long and wrapping was required. Names like 
`shutdown` should work, but this will be inconsistent with other code in this 
file.


- Dmitry


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


On Nov. 21, 2017, 5:53 p.m., Dmitry Zhuk wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63914/
> ---
> 
> (Updated Nov. 21, 2017, 5:53 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Michael Park.
> 
> 
> Bugs: MESOS-6972
> https://issues.apache.org/jira/browse/MESOS-6972
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `reregisterSlave` now accepts `ReregisterSlaveMessage&&`, which opts-out
> of using protobuf arena, and allows passing message through dispatch
> chain without making any copies.
> Conversion of repeated message fields to `std::vector`s is performed
> only when needed.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 0f8a2ac72c3484f911853c2994fc71a488d66d96 
>   src/master/master.cpp fadc78b2ca5d46b8cc12a794b428753aa79ac095 
>   src/master/validation.hpp ac54062ea09f97ad96bd17deb106ea89a57f394a 
>   src/master/validation.cpp 8b5848bfd8c069f34a92a9a68597955c6e0c2ee2 
>   src/tests/master_validation_tests.cpp 
> 0e1c8b490ebe10bc50b8b6b530fa85128b967584 
> 
> 
> Diff: https://reviews.apache.org/r/63914/diff/4/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Dmitry Zhuk
> 
>

Re: Review Request 63915: Reduced tasks copying during agent reregistration.

[Dmitry Zhuk]

> On Dec. 4, 2017, 7:37 p.m., Michael Park wrote:
> > src/master/master.hpp
> > Line 128 (original), 128 (patched)
> > 
> >
> > Maybe we can consider making this `vector&&` as discussed in 
> > https://reviews.apache.org/r/63914/

This will make some difficulties for `registerSlave` which also uses this 
constructor. Shall we migrate `registerSlave` to using message similar to 
`reregisterSlave`?


- Dmitry


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


On Nov. 21, 2017, 5:53 p.m., Dmitry Zhuk wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63915/
> ---
> 
> (Updated Nov. 21, 2017, 5:53 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Michael Park.
> 
> 
> Bugs: MESOS-6972
> https://issues.apache.org/jira/browse/MESOS-6972
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Tasks can be moved into master's internal data structures from message
> to save some cycles on copying the data.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 0f8a2ac72c3484f911853c2994fc71a488d66d96 
>   src/master/master.cpp fadc78b2ca5d46b8cc12a794b428753aa79ac095 
> 
> 
> Diff: https://reviews.apache.org/r/63915/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> Ran benchmark with `--enable-optimize --enable-lock-free-run-queue 
> --enable-lock-free-event-queue 
> --enable-last-in-first-out-fixed-size-semaphore`
> `./mesos-tests.sh --benchmark 
> --gtest_filter=AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.*`
> On 2608c0b8f62a9359d3d23e1724b6e91f316cfc76 (includes protobuf-3.5.0):
> ```
> [ RUN  ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/0
> Starting reregistration for all agents
> Reregistered 2000 agents with a total of 10 running tasks and 10 
> completed tasks in 16.202206916secs
> [   OK ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/0
>  (30065 ms)
> [ RUN  ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/1
> Starting reregistration for all agents
> Reregistered 2000 agents with a total of 20 running tasks and 0 completed 
> tasks in 30.509804836secs
> [   OK ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/1
>  (57145 ms)
> [ RUN  ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/2
> Starting reregistration for all agents
> Reregistered 2 agents with a total of 10 running tasks and 0 
> completed tasks in 22.581999748secs
> [   OK ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/2
>  (39629 ms)
> ```
> 
> On this chain of patches:
> ```
> [ RUN  ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/0
> Starting reregistration for all agents
> Reregistered 2000 agents with a total of 10 running tasks and 10 
> completed tasks in 8.456615936secs
> [   OK ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/0
>  (22659 ms)
> [ RUN  ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/1
> Starting reregistration for all agents
> Reregistered 2000 agents with a total of 20 running tasks and 0 completed 
> tasks in 15.09102354secs
> [   OK ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/1
>  (43828 ms)
> [ RUN  ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/2
> Starting reregistration for all agents
> Reregistered 2 agents with a total of 10 running tasks and 0 
> completed tasks in 16.122729767secs
> [   OK ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/2
>  (33182 ms)
> ```
> 
> 
> Thanks,
> 
> Dmitry Zhuk
> 
>

Re: Review Request 64151: Added a V1 API call to list resource providers.

[Jan Schlicht]

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

(Updated Dec. 5, 2017, 3:34 p.m.)


Review request for mesos, Benjamin Bannier and Jie Yu.


Changes
---

Addressed issues.


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


Repository: mesos


Description
---

The 'GET_RESOURCE_PROVIDERS' call will list all subscribed local
resource providers of an agent.


Diffs (updated)
-

  include/mesos/agent/agent.proto 0f92f73ba0f7729f0ba7cd89a692ab3685125e8b 
  include/mesos/v1/agent/agent.proto 012ffef5f0dd7720fa95ae484c99479aaf256d7b 
  src/resource_provider/manager.hpp c2aeb15b0b8ebd167ddf9c42c9fc396d4c0a126b 
  src/resource_provider/manager.cpp 8d8b2f1396230d6edba590b8b7e9b4ca51366efe 
  src/slave/http.hpp a51831cdcebc1998ce6d4c3c19285e598a4ec9a3 
  src/slave/http.cpp fd0e809d5dc7722e573b66621f75d791f0911dfa 
  src/slave/validation.cpp 32781fd8f124f71e61744804aec3fe4da59a5df2 
  src/tests/api_tests.cpp 53c705ed4775846ba7eb42da57f962895f5669aa 
  src/tests/resource_provider_manager_tests.cpp 
a4c19ca769e66110d9aba0bae4792df9db3fed01 


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

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


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 63022: Imported and reconcile resources from CSI plugins.

[James DeFelice]

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




src/resource_provider/storage/provider.cpp
Lines 1203 (patched)


some plugins might not implement create/delete, but DO implement list. such 
a plugin might only support "block"- (or only support "mount"-) type volumes. 
in such cases, it could be useful to report those resources as BLOCK (or MOUNT) 
volumes instead of RAW. I don't *think* it's a blocker for MVP, but we've had a 
couple of conversations recently about exposing low-level primitives to 
specialized storage frameworks, and that it might be useful to report block 
devices as BLOCK disk resources, by default.


- James DeFelice


On Dec. 5, 2017, 5:57 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63022/
> ---
> 
> (Updated Dec. 5, 2017, 5:57 a.m.)
> 
> 
> Review request for mesos, James DeFelice, Jie Yu, Joseph Wu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-8101
> https://issues.apache.org/jira/browse/MESOS-8101
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The following lists the steps to reconcile resources:
> 
> 1. Import resources from the CSI plugin:
>   a. Get preprovisioned volumes:
>ListVolumes
>ValidateVolumeCapabilities for each volume
>   b. GetCapacity for each profile to get RAW resources
> 2. For each resource in the checkpointed resources:
>   a. Strip away metadata (reservation, persistence, etc)
>   b. Additional resources after subtracting the stripped resources
>  from the imported resources are new resources.
> 3. Report the reconciled resources through UPDATE_STATE
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> d35b0d02992e3730ca47906b34c21e1ba9c653e7 
> 
> 
> Diff: https://reviews.apache.org/r/63022/diff/16/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 63632: Migrated to event consumer interface.

[Dmitry Zhuk]

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

(Updated Dec. 5, 2017, 1:50 p.m.)


Review request for mesos, Benjamin Mahler and Michael Park.


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


Repository: mesos


Description
---

Migrated to event consumer interface.


Diffs (updated)
-

  src/common/recordio.hpp 378363492e04c141671fbed0467bf558b61e5dae 
  src/master/master.hpp 448be3b4a3eb3d0cbd34a39b19640d15cf3fc2af 
  src/master/master.cpp 31991b5f7f7f4e7dc73d71a3160348bfc0a627c3 
  src/tests/fetcher_cache_tests.cpp e2ecb102e2e7b4c4b3b2b736fdc453feeb15816e 


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

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


Testing
---

make check


Thanks,

Dmitry Zhuk

Re: Review Request 64151: Added a V1 API call to list resource providers.

[Benjamin Bannier]

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




include/mesos/agent/agent.proto
Lines 445 (patched)


Let's introduce a nested message `GetResourceProviders.ResourceProvider` 
here so that we can add more information later (e.g., resource 
provider-specific resources, offer operations, ...). This message would contain 
just a `repeated ResourceProvider resource_providers = 1` for now.



include/mesos/v1/agent/agent.proto
Lines 445 (patched)


Ditto.


- Benjamin Bannier


On Dec. 5, 2017, 10:48 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64151/
> ---
> 
> (Updated Dec. 5, 2017, 10:48 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Bugs: MESOS-8270
> https://issues.apache.org/jira/browse/MESOS-8270
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The 'GET_RESOURCE_PROVIDERS' call will list all subscribed local
> resource providers of an agent.
> 
> 
> Diffs
> -
> 
>   include/mesos/agent/agent.proto 0f92f73ba0f7729f0ba7cd89a692ab3685125e8b 
>   include/mesos/v1/agent/agent.proto 012ffef5f0dd7720fa95ae484c99479aaf256d7b 
>   src/resource_provider/manager.hpp c2aeb15b0b8ebd167ddf9c42c9fc396d4c0a126b 
>   src/resource_provider/manager.cpp 8d8b2f1396230d6edba590b8b7e9b4ca51366efe 
>   src/slave/http.hpp a51831cdcebc1998ce6d4c3c19285e598a4ec9a3 
>   src/slave/http.cpp fd0e809d5dc7722e573b66621f75d791f0911dfa 
>   src/slave/validation.cpp 32781fd8f124f71e61744804aec3fe4da59a5df2 
>   src/tests/api_tests.cpp 53c705ed4775846ba7eb42da57f962895f5669aa 
>   src/tests/resource_provider_manager_tests.cpp 
> a4c19ca769e66110d9aba0bae4792df9db3fed01 
> 
> 
> Diff: https://reviews.apache.org/r/64151/diff/5/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 64101: Skip registry update when nothing changed.

[Benno Evers]

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

(Updated Dec. 5, 2017, 1:36 p.m.)


Review request for mesos and Vinod Kone.


Repository: mesos


Description
---

Most of the time, agents will not change their configuration
when they restart, so we can skip updating the registry in this
case.


Diffs (updated)
-

  src/master/master.cpp 883793a226849434eb833c3027d121635a86fdc4 


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

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


Testing
---

`make check`

https://jenkins.mesosphere.com/service/jenkins/view/Mesos/job/mesos/job/Mesos_CI-build/2302/


Thanks,

Benno Evers

Re: Review Request 64247: Fixed a flaky test case in reservation tests.

[Jan Schlicht]

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

(Updated Dec. 5, 2017, 2:05 p.m.)


Review request for mesos, Alexander Rukletsov, Benjamin Bannier, and Jie Yu.


Changes
---

Rebased, removed superfluous code.


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


Repository: mesos


Description (updated)
---

ReservationTest.MasterFailover could fail when parameterized with
the 'RESOURCE_PROVIDER' capability. After the simulated master failover,
the re-registered framework could receive an offer before the
re-registered agent sends a 'UPDATE_SLAVE' message. On receiving this
message, the master would rescind the offer and send out a new one.
The test expected a single offer to be send. It has been changed to
accept the first offer and ignore subsequent ones.

Also, the test case has been updated to run with paused clocks.


Diffs (updated)
-

  src/tests/reservation_tests.cpp 7f1bb398c33b3b8cad371b49bdc4bc2454f6440d 


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

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


Testing
---

src/mesos-tests

`src/mesos-tests 
--gtest_filter=ResourceProviderCapability/ReservationTest.MasterFailover/1 
--gtest_repeat=2000 --gtest_break_on_failure` while running `stress --cpu 8 
--io 8` in the background (on a 4 core machine)


Thanks,

Jan Schlicht

Re: Review Request 63637: Added callable once support in defer.

[Dmitry Zhuk]

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

(Updated Dec. 5, 2017, 12:20 p.m.)


Review request for mesos, Benjamin Mahler and Michael Park.


Changes
---

Removed use of `lambda::Placeholder`.


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


Repository: mesos


Description
---

This allows `defer` result to be converted to `CallableOnce`, ensuring
that bound arguments are moved, when call is made, and avoiding making
copies of bound arguments.


Diffs (updated)
-

  3rdparty/libprocess/include/process/deferred.hpp 
950f1cccaf93298664a95ba1fd2c5d2a42deb725 
  3rdparty/libprocess/src/tests/process_tests.cpp 
d1b30b0754c55fd765885ddcfe9f9f68ba509ac1 


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

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


Testing
---

make check


Thanks,

Dmitry Zhuk

Review Request 64338: Reduced number of supported arguments in _Deferred conversion operators.

[Dmitry Zhuk]

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

Review request for mesos, Benjamin Mahler and Michael Park.


Repository: mesos


Description
---

Conversion of `_Deferred` to `std::function` and `Deferred` currently
supports up to 12 parameters in function signature. However, this is
unnecessary and is a huge overhead. Most usages require just one
parameter (e.g. when `defer` is used with `Future`). And there are few
usages with two parameters (in `master.cpp` to initialize allocator, and
in `slave.cpp` to install signal handler).
This number of parameters is different from the number of parameters
passed to `defer`, but it's related and can be defined as maximum number
of placeholders that can be passed to `defer`.

Given that `deferred.hpp` is indirectly included in most source files,
it is beneficial to keep this number low. This patch changes maximum
number of parameters to 2.


Diffs
-

  3rdparty/libprocess/include/process/deferred.hpp 
950f1cccaf93298664a95ba1fd2c5d2a42deb725 


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


Testing
---

make check


Thanks,

Dmitry Zhuk

Review Request 64337: Fixed CallableOnce operator() signature.

[Dmitry Zhuk]

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

Review request for mesos, Benjamin Mahler and Michael Park.


Repository: mesos


Description
---

This changes `operator()` signature to match the one defined in
`CallableOnce` template parameter.
Previously used form incorrectly specifies that `operator()` can be
invoked with arbitrary number and types of parameters, which can break
other templates using SFINAE to check if functor can be invoked with
specific parameters.


Diffs
-

  3rdparty/stout/include/stout/lambda.hpp 
96586bd572093d2874a617252ed22b9e6874236b 


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


Testing
---

make check


Thanks,

Dmitry Zhuk

Re: Review Request 64247: Fixed a flaky test case in reservation tests.

[Jan Schlicht]

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

(Updated Dec. 5, 2017, 11:17 a.m.)


Review request for mesos, Alexander Rukletsov, Benjamin Bannier, and Jie Yu.


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


Repository: mesos


Description (updated)
---

ReservationTest.MasterFailover could fail when parameterized with
the 'RESOURCE_PROVIDER' capability. After the simulated master
failover, the re-registered framework could receive an offer before
the re-registered agent sends a 'UPDATE_SLAVE' message. On receiving
this message, the master would rescind the offer and send out a new
one. The test expected a single offer to be send. It has been changed
to accept the first offer and ignore subsequent ones.

Also, the test case has been updated to run with paused clocks.


Diffs
-

  src/tests/reservation_tests.cpp f29e9bbb3d3b4dbaf21cd04f5efcf0df7cc34459 


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


Testing
---

src/mesos-tests

`src/mesos-tests 
--gtest_filter=ResourceProviderCapability/ReservationTest.MasterFailover/1 
--gtest_repeat=2000 --gtest_break_on_failure` while running `stress --cpu 8 
--io 8` in the background (on a 4 core machine)


Thanks,

Jan Schlicht

Re: Review Request 64247: Fixed a flaky test case in reservation tests.

[Jan Schlicht]

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

(Updated Dec. 5, 2017, 12:15 p.m.)


Review request for mesos, Alexander Rukletsov, Benjamin Bannier, and Jie Yu.


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


Repository: mesos


Description (updated)
---

ReservationTest.MasterFailover could fail when parameterized with
the 'RESOURCE_PROVIDER' capability. After the simulated master failover,
the re-registered framework could receive an offer before the
re-registered agent sends a 'UPDATE_SLAVE' message. On receiving this message,
the master would rescind the offer and send out a new one. The test
expected a single offer to be send. It has been changed to accept the
first offer and ignore subsequent ones.

Also, the test case has been updated to run with paused clocks.


Diffs
-

  src/tests/reservation_tests.cpp f29e9bbb3d3b4dbaf21cd04f5efcf0df7cc34459 


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


Testing
---

src/mesos-tests

`src/mesos-tests 
--gtest_filter=ResourceProviderCapability/ReservationTest.MasterFailover/1 
--gtest_repeat=2000 --gtest_break_on_failure` while running `stress --cpu 8 
--io 8` in the background (on a 4 core machine)


Thanks,

Jan Schlicht

Review Request 64335: Made "agent_features" flag available on non-Linux systems.

[Jan Schlicht]

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

Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, and Jie Yu.


Repository: mesos


Description
---

The "agent_features" was implemented in a block that was only compiled
on Linux. As it should be available on every target system, the
implementation has been move out of the block.


Diffs
-

  src/slave/flags.cpp 34edf4cb4a1afc366e6b6b4ddf78e90e49853fdf 


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


Testing
---

make check

Ran `src/mesos-agent --help` on macOS and checked that the `agent_feature` flag 
is listed there.


Thanks,

Jan Schlicht

Re: Review Request 64151: Added a V1 API call to list resource providers.

[Jan Schlicht]

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

(Updated Dec. 5, 2017, 10:48 a.m.)


Review request for mesos, Benjamin Bannier and Jie Yu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

The 'GET_RESOURCE_PROVIDERS' call will list all subscribed local
resource providers of an agent.


Diffs (updated)
-

  include/mesos/agent/agent.proto 0f92f73ba0f7729f0ba7cd89a692ab3685125e8b 
  include/mesos/v1/agent/agent.proto 012ffef5f0dd7720fa95ae484c99479aaf256d7b 
  src/resource_provider/manager.hpp c2aeb15b0b8ebd167ddf9c42c9fc396d4c0a126b 
  src/resource_provider/manager.cpp 8d8b2f1396230d6edba590b8b7e9b4ca51366efe 
  src/slave/http.hpp a51831cdcebc1998ce6d4c3c19285e598a4ec9a3 
  src/slave/http.cpp fd0e809d5dc7722e573b66621f75d791f0911dfa 
  src/slave/validation.cpp 32781fd8f124f71e61744804aec3fe4da59a5df2 
  src/tests/api_tests.cpp 53c705ed4775846ba7eb42da57f962895f5669aa 
  src/tests/resource_provider_manager_tests.cpp 
a4c19ca769e66110d9aba0bae4792df9db3fed01 


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

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


Testing
---

make check


Thanks,

Jan Schlicht

Review Request 64332: Removed `constexpr`-ness from `cpp17::invoke`.

[Michael Park]

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

Review request for mesos and Benjamin Bannier.


Repository: mesos


Description
---

`std::invoke` is not marked `constexpr`.


Diffs
-

  3rdparty/stout/include/stout/cpp17.hpp 
3c4cc5345c94270785fa1594d7310af2b4dcf402 


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


Testing
---


Thanks,

Michael Park

Review Request 64331: Added more `cpp17::invoke` test cases.

[Michael Park]

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

Review request for mesos and Benjamin Bannier.


Repository: mesos


Description
---

See summary.


Diffs
-

  3rdparty/stout/tests/cpp17_tests.cpp 37d817f1af90cc6a17c5d7f17cf0571fd3f4ac7c 


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


Testing
---


Thanks,

Michael Park