Re: Review Request 64054: Refactored offer operation handling for speculative operations.

[Jie Yu]

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


Fix it, then Ship it!





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


This is unnecessary. You just created the offer operation above with 
OFFER_OPERATION_PENDING. If you really want, this could be a CHECK.


- Jie Yu


On Nov. 28, 2017, 2:25 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64054/
> ---
> 
> (Updated Nov. 28, 2017, 2:25 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Applying operations has been refactored out of 'addOfferOperation' and
> simplified by using 'protobuf::isSpeculativeOperation'.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 53263e499d88b906b6406c24c0dfb737e589e813 
>   src/slave/slave.cpp 6ed5c7887cf998b92cf3181b29cb6cf09cc73e61 
> 
> 
> Diff: https://reviews.apache.org/r/64054/diff/4/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 64054: Refactored offer operation handling for speculative operations.

[Jan Schlicht]

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

(Updated Nov. 28, 2017, 3:25 p.m.)


Review request for mesos, Benjamin Bannier and Jie Yu.


Changes
---

Addressed Benjamin's issue.


Repository: mesos


Description
---

Applying operations has been refactored out of 'addOfferOperation' and
simplified by using 'protobuf::isSpeculativeOperation'.


Diffs (updated)
-

  src/master/master.cpp 53263e499d88b906b6406c24c0dfb737e589e813 
  src/slave/slave.cpp 6ed5c7887cf998b92cf3181b29cb6cf09cc73e61 


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

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


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 64054: Refactored offer operation handling for speculative operations.

[Benjamin Bannier]

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




src/master/master.cpp
Line 10867 (original), 10879 (patched)


We should only count non-terminal operations here. Could you update the 
condition?


- Benjamin Bannier


On Nov. 28, 2017, 11:42 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64054/
> ---
> 
> (Updated Nov. 28, 2017, 11:42 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Applying operations has been refactored out of 'addOfferOperation' and
> simplified by using 'protobuf::isSpeculativeOperation'.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 53263e499d88b906b6406c24c0dfb737e589e813 
>   src/slave/slave.cpp 6ed5c7887cf998b92cf3181b29cb6cf09cc73e61 
> 
> 
> Diff: https://reviews.apache.org/r/64054/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 64054: Refactored offer operation handling for speculative operations.

[Jan Schlicht]

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

(Updated Nov. 28, 2017, 11:42 a.m.)


Review request for mesos, Benjamin Bannier and Jie Yu.


Changes
---

Addressed issues.


Repository: mesos


Description
---

Applying operations has been refactored out of 'addOfferOperation' and
simplified by using 'protobuf::isSpeculativeOperation'.


Diffs (updated)
-

  src/master/master.cpp 53263e499d88b906b6406c24c0dfb737e589e813 
  src/slave/slave.cpp 6ed5c7887cf998b92cf3181b29cb6cf09cc73e61 


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

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


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 64054: Refactored offer operation handling for speculative operations.

[Benjamin Bannier]

> On Nov. 26, 2017, 4:40 a.m., Jie Yu wrote:
> > src/master/master.hpp
> > Lines 2795-2841 (original)
> > 
> >
> > This should still be part of `addOfferOperation`. In fact, anything to 
> > do with `used` resourecs should still be part of `addOfferOperation`.
> > 
> > Only the part that's converting `total` resources should be pulled out 
> > because the way we change total does not rely on each offer operation. 
> > instead, we rely on the snapshot value received in `UpdateSlaveMessage`.
> 
> Benjamin Bannier wrote:
> While we do not strictly need to remove the updates to `used` here, I 
> feel it might be worthwhile to nevertheless completely remove any updates to 
> resource state here, and instead make this function operate exclusively on 
> offer operation state (and additionally in the future e.g., trigger offer 
> operation status reconciliation). To me this seems like a natural division of 
> responsibilities to me.

Discussed this offline with Jie. A strong argument in favor of still 
manipulating the used resources here is consistency with how tasks are 
currently handled.


- Benjamin


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


On Nov. 24, 2017, 12:24 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64054/
> ---
> 
> (Updated Nov. 24, 2017, 12:24 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Applying operations has been refactored out of 'addOfferOperation' and
> simplified by using 'protobuf::isSpeculativeOperation'.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 2a2e830354db4a2191fb8321beb8174b80f7ba7d 
>   src/master/master.cpp 53263e499d88b906b6406c24c0dfb737e589e813 
>   src/slave/slave.cpp f93ff7b20815c3ccb274ce6990ee66a17b6ac51c 
> 
> 
> Diff: https://reviews.apache.org/r/64054/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 64054: Refactored offer operation handling for speculative operations.

[Benjamin Bannier]

> On Nov. 26, 2017, 4:40 a.m., Jie Yu wrote:
> > src/master/master.hpp
> > Lines 2795-2841 (original)
> > 
> >
> > This should still be part of `addOfferOperation`. In fact, anything to 
> > do with `used` resourecs should still be part of `addOfferOperation`.
> > 
> > Only the part that's converting `total` resources should be pulled out 
> > because the way we change total does not rely on each offer operation. 
> > instead, we rely on the snapshot value received in `UpdateSlaveMessage`.

While we do not strictly need to remove the updates to `used` here, I feel it 
might be worthwhile to nevertheless completely remove any updates to resource 
state here, and instead make this function operate exclusively on offer 
operation state (and additionally in the future e.g., trigger offer operation 
status reconciliation). To me this seems like a natural division of 
responsibilities to me.


- Benjamin


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


On Nov. 24, 2017, 12:24 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64054/
> ---
> 
> (Updated Nov. 24, 2017, 12:24 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Applying operations has been refactored out of 'addOfferOperation' and
> simplified by using 'protobuf::isSpeculativeOperation'.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 2a2e830354db4a2191fb8321beb8174b80f7ba7d 
>   src/master/master.cpp 53263e499d88b906b6406c24c0dfb737e589e813 
>   src/slave/slave.cpp f93ff7b20815c3ccb274ce6990ee66a17b6ac51c 
> 
> 
> Diff: https://reviews.apache.org/r/64054/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 64054: Refactored offer operation handling for speculative operations.

[Jie Yu]

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




src/master/master.hpp
Lines 2795-2841 (original)


This should still be part of `addOfferOperation`. In fact, anything to do 
with `used` resourecs should still be part of `addOfferOperation`.

Only the part that's converting `total` resources should be pulled out 
because the way we change total does not rely on each offer operation. instead, 
we rely on the snapshot value received in `UpdateSlaveMessage`.



src/master/master.cpp
Lines 8608-8633 (patched)


See comments above. THis is not necessary.



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


This is not necessary. You just need to pull the `slave->apply` part this 
this function.



src/master/master.cpp
Lines 10878-10893 (original)


You just need to adjust this part. The rest should not be touched.


- Jie Yu


On Nov. 24, 2017, 11:24 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64054/
> ---
> 
> (Updated Nov. 24, 2017, 11:24 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Applying operations has been refactored out of 'addOfferOperation' and
> simplified by using 'protobuf::isSpeculativeOperation'.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 2a2e830354db4a2191fb8321beb8174b80f7ba7d 
>   src/master/master.cpp 53263e499d88b906b6406c24c0dfb737e589e813 
>   src/slave/slave.cpp f93ff7b20815c3ccb274ce6990ee66a17b6ac51c 
> 
> 
> Diff: https://reviews.apache.org/r/64054/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 64054: Refactored offer operation handling for speculative operations.

[Jan Schlicht]

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

(Updated Nov. 24, 2017, 12:24 p.m.)


Review request for mesos, Benjamin Bannier and Jie Yu.


Changes
---

Also refactored `addOfferOperation` in `master.cpp`.


Repository: mesos


Description
---

Applying operations has been refactored out of 'addOfferOperation' and
simplified by using 'protobuf::isSpeculativeOperation'.


Diffs (updated)
-

  src/master/master.hpp 2a2e830354db4a2191fb8321beb8174b80f7ba7d 
  src/master/master.cpp 53263e499d88b906b6406c24c0dfb737e589e813 
  src/slave/slave.cpp f93ff7b20815c3ccb274ce6990ee66a17b6ac51c 


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

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


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 64054: Refactored offer operation handling for speculative operations.

[Benjamin Bannier]

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



I believe we need a similar change in master's `Slave::addOfferOperation`.

- Benjamin Bannier


On Nov. 23, 2017, 4:39 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64054/
> ---
> 
> (Updated Nov. 23, 2017, 4:39 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Applying operations has been refactored out of 'addOfferOperation' and
> simplified by using 'protobuf::isSpeculativeOperation'.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp f93ff7b20815c3ccb274ce6990ee66a17b6ac51c 
> 
> 
> Diff: https://reviews.apache.org/r/64054/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>