Re: Review Request 63620: Updated offer operation handling to set resource versions.

2017-11-08 Thread Jie Yu 

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


Fix it, then Ship it!





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


I would call this `resourceVersion` to be consistent with below.



src/master/master.cpp
Lines 5393-5400 (patched)


I'd just use CHECK here (to be consistent with that in `_apply`)


- Jie Yu


On Nov. 8, 2017, 3:23 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63620/
> ---
> 
> (Updated Nov. 8, 2017, 3:23 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Resource version UUIDs are used to establish a relationship between
> operation and the resource they are operating on. For each agent the
> master keeps track of its resource version and the resource version of
> the agents' resource providers. As resource versions are required in a
> 'ApplyOfferOperationMessage', the master has to find the resource
> version that's matching the resources an operation should apply to and
> set it accordingly.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp ee212c1a21b432d3aa8e90d748d12cb0a754d57c 
> 
> 
> Diff: https://reviews.apache.org/r/63620/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 63620: Updated offer operation handling to set resource versions.

2017-11-08 Thread Jan Schlicht 

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

(Updated Nov. 8, 2017, 4:23 p.m.)


Review request for mesos, Benjamin Bannier and Jie Yu.


Changes
---

Addressed issues.


Repository: mesos


Description
---

Resource version UUIDs are used to establish a relationship between
operation and the resource they are operating on. For each agent the
master keeps track of its resource version and the resource version of
the agents' resource providers. As resource versions are required in a
'ApplyOfferOperationMessage', the master has to find the resource
version that's matching the resources an operation should apply to and
set it accordingly.


Diffs (updated)
-

  src/master/master.cpp ee212c1a21b432d3aa8e90d748d12cb0a754d57c 


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

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


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 63620: Updated offer operation handling to set resource versions.

2017-11-07 Thread Jie Yu 

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




src/master/master.cpp
Line 9905 (original), 9969 (patched)


I think `checkpointedResources` should be limited to agent default 
resources. So we might want to tweak `needCheckpointing` filter.

Also, I think we probably should consider using `_apply` for new operations 
too.

With the above in mind, here, we probably need to adjust the code to only 
call `slave->apply(operation)` when the opreation is old operaitons because for 
new operations, slave's total doesn't change until terminal status update.



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


I'd prefer we use CHECK here and move the validation to caller (and drop 
there). And I am pretty sure this is validated in master operation validation.

Utility functions like this should be kept without too many branches.



src/master/master.cpp
Lines 9980-9985 (patched)


YOu can use the following:
```
Option resourceVersion = resourceProviderId.isSome()
  ? slave->resourceVersions.get(resourceProviderId.get())
  : slave->resourceVersions.get(None());
```



src/master/master.cpp
Lines 9987-9995 (patched)


I would just use a CHECK_SOME here.


- Jie Yu


On Nov. 7, 2017, 2:18 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63620/
> ---
> 
> (Updated Nov. 7, 2017, 2:18 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Resource version UUIDs are used to establish a relationship between
> operation and the resource they are operating on. For each agent the
> master keeps track of its resource version and the resource version of
> the agents' resource providers. As resource versions are required in a
> 'ApplyOfferOperationMessage', the master has to find the resource
> version that's matching the resources an operation should apply to and
> set it accordingly.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp ee212c1a21b432d3aa8e90d748d12cb0a754d57c 
> 
> 
> Diff: https://reviews.apache.org/r/63620/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 63620: Updated offer operation handling to set resource versions.

2017-11-07 Thread Benjamin Bannier 

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




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


nit: Do we require the `Some` here? I would have guessed that this would 
implicitly construct some provider id since `Option`'s constructor is implicit.



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


nit: `Some` is not needed here.



src/master/master.cpp
Lines 9987-9995 (patched)


Since we `apply` above, we here need to instead have a hard 
`CHECK_SOME(uuid)`, rollback, or apply later.


- Benjamin Bannier


On Nov. 7, 2017, 3:18 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63620/
> ---
> 
> (Updated Nov. 7, 2017, 3:18 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Resource version UUIDs are used to establish a relationship between
> operation and the resource they are operating on. For each agent the
> master keeps track of its resource version and the resource version of
> the agents' resource providers. As resource versions are required in a
> 'ApplyOfferOperationMessage', the master has to find the resource
> version that's matching the resources an operation should apply to and
> set it accordingly.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp ee212c1a21b432d3aa8e90d748d12cb0a754d57c 
> 
> 
> Diff: https://reviews.apache.org/r/63620/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Review Request 63620: Updated offer operation handling to set resource versions.

2017-11-07 Thread Jan Schlicht 

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

Review request for mesos, Benjamin Bannier and Jie Yu.


Repository: mesos


Description
---

Resource version UUIDs are used to establish a relationship between
operation and the resource they are operating on. For each agent the
master keeps track of its resource version and the resource version of
the agents' resource providers. As resource versions are required in a
'ApplyOfferOperationMessage', the master has to find the resource
version that's matching the resources an operation should apply to and
set it accordingly.


Diffs
-

  src/master/master.cpp ee212c1a21b432d3aa8e90d748d12cb0a754d57c 


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


Testing
---

make check


Thanks,

Jan Schlicht