Re: Review Request 69858: Persisted intentionally dropped operations in SLRP.

[Benjamin Bannier]

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


Ship it!




Ship It!

- Benjamin Bannier


On Feb. 6, 2019, 1:04 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69858/
> ---
> 
> (Updated Feb. 6, 2019, 1:04 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gastón Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-9537
> https://issues.apache.org/jira/browse/MESOS-9537
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If an operation is dropped intentionally (e.g., because of a resource
> version mismatch), the operation should be persisted so no conflicting
> status update would be generated for operation reconciliation.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> d6e20a549ede189c757ae3ae922ab7cb86d2be2c 
>   src/resource_provider/storage/provider_process.hpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/69858/diff/4/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Additional test MESOS-9537 is added later in chain.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 69858: Persisted intentionally dropped operations in SLRP.

[Chun-Hung Hsiao]

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

(Updated Feb. 6, 2019, 12:04 a.m.)


Review request for mesos, Benjamin Bannier, Gastón Kleiman, and Greg Mann.


Changes
---

Addressed Benjamin's comments and fixed a bug.


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


Repository: mesos


Description
---

If an operation is dropped intentionally (e.g., because of a resource
version mismatch), the operation should be persisted so no conflicting
status update would be generated for operation reconciliation.


Diffs (updated)
-

  src/resource_provider/storage/provider.cpp 
d6e20a549ede189c757ae3ae922ab7cb86d2be2c 
  src/resource_provider/storage/provider_process.hpp PRE-CREATION 


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

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


Testing
---

`make check`

Additional test MESOS-9537 is added later in chain.


Thanks,

Chun-Hung Hsiao

Re: Review Request 69858: Persisted intentionally dropped operations in SLRP.

[Chun-Hung Hsiao]

> On Feb. 5, 2019, 12:08 p.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 2889 (patched)
> > 
> >
> > Why is this always true? Probably a good idea to document this here if 
> > we make this assertion. I'd suggest to not make this assertion and instead 
> > simplify the control flow, see below.

Hmm good point. If we support operator API in the future than this assertion 
won't hold.


> On Feb. 5, 2019, 12:08 p.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 2888-2892 (original), 2898-2910 (patched)
> > 
> >
> > What do you think about constructing a single update outside of any 
> > condition (like was done previously), and then only have branching 
> > depending on whether we want to use the status update manager or send 
> > unreliably?
> > 
> > I think this would read much better if we'd:
> > * gather all required input to create the status update in variables,
> > * construct the status update, and
> > * a single `if/else` for the sending.
> > * update metrics in a single place

I come up with a slightly different refactor. Please give feedback if it's 
still not readable :) Leaving this open for now.


> On Feb. 5, 2019, 12:08 p.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 2923 (patched)
> > 
> >
> > We could introduce a variable for this.

Resolved through a different refactor. Closing.


- Chun-Hung


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


On Jan. 30, 2019, 10:35 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69858/
> ---
> 
> (Updated Jan. 30, 2019, 10:35 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gastón Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-9537
> https://issues.apache.org/jira/browse/MESOS-9537
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If an operation is dropped intentionally (e.g., because of a resource
> version mismatch), the operation should be persisted so no conflicting
> status update would be generated for operation reconciliation.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> d6e20a549ede189c757ae3ae922ab7cb86d2be2c 
>   src/resource_provider/storage/provider_process.hpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/69858/diff/3/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Additional test MESOS-9537 is added later in chain.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 69858: Persisted intentionally dropped operations in SLRP.

[Benjamin Bannier]

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




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


Why is this always true? Probably a good idea to document this here if we 
make this assertion. I'd suggest to not make this assertion and instead 
simplify the control flow, see below.



src/resource_provider/storage/provider.cpp
Lines 2888-2892 (original), 2898-2910 (patched)


What do you think about constructing a single update outside of any 
condition (like was done previously), and then only have branching depending on 
whether we want to use the status update manager or send unreliably?

I think this would read much better if we'd:
* gather all required input to create the status update in variables,
* construct the status update, and
* a single `if/else` for the sending.
* update metrics in a single place



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


We could introduce a variable for this.



src/resource_provider/storage/provider_process.hpp
Line 298 (original), 298-300 (patched)


It isn't clear from the second sentence whether you document behavior or 
give a prescription on how to use this function.


- Benjamin Bannier


On Jan. 30, 2019, 11:35 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69858/
> ---
> 
> (Updated Jan. 30, 2019, 11:35 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gastón Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-9537
> https://issues.apache.org/jira/browse/MESOS-9537
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If an operation is dropped intentionally (e.g., because of a resource
> version mismatch), the operation should be persisted so no conflicting
> status update would be generated for operation reconciliation.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> d6e20a549ede189c757ae3ae922ab7cb86d2be2c 
>   src/resource_provider/storage/provider_process.hpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/69858/diff/3/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Additional test MESOS-9537 is added later in chain.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 69858: Persisted intentionally dropped operations in SLRP.

[Chun-Hung Hsiao]

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

(Updated Jan. 30, 2019, 10:35 p.m.)


Review request for mesos, Benjamin Bannier, Gastón Kleiman, and Greg Mann.


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


Repository: mesos


Description
---

If an operation is dropped intentionally (e.g., because of a resource
version mismatch), the operation should be persisted so no conflicting
status update would be generated for operation reconciliation.


Diffs
-

  src/resource_provider/storage/provider.cpp 
d6e20a549ede189c757ae3ae922ab7cb86d2be2c 
  src/resource_provider/storage/provider_process.hpp PRE-CREATION 


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


Testing (updated)
---

`make check`

Additional test MESOS-9537 is added later in chain.


Thanks,

Chun-Hung Hsiao

Re: Review Request 69858: Persisted intentionally dropped operations in SLRP.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 69858 was successfully built and tested.

Reviews applied: `['69858']`

All the build artifacts available at: 
http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2835/mesos-review-69858

- Mesos Reviewbot Windows


On Jan. 30, 2019, 9:32 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69858/
> ---
> 
> (Updated Jan. 30, 2019, 9:32 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gastón Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-9537
> https://issues.apache.org/jira/browse/MESOS-9537
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If an operation is dropped intentionally (e.g., because of a resource
> version mismatch), the operation should be persisted so no conflicting
> status update would be generated for operation reconciliation.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> d6e20a549ede189c757ae3ae922ab7cb86d2be2c 
>   src/resource_provider/storage/provider_process.hpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/69858/diff/3/
> 
> 
> Testing
> ---
> 
> Test will be added later in chain.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 69858: Persisted intentionally dropped operations in SLRP.

[Chun-Hung Hsiao]

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

(Updated Jan. 30, 2019, 8:32 p.m.)


Review request for mesos, Benjamin Bannier, Gastón Kleiman, and Greg Mann.


Changes
---

Fixed a test issue.


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


Repository: mesos


Description
---

If an operation is dropped intentionally (e.g., because of a resource
version mismatch), the operation should be persisted so no conflicting
status update would be generated for operation reconciliation.


Diffs (updated)
-

  src/resource_provider/storage/provider.cpp 
d6e20a549ede189c757ae3ae922ab7cb86d2be2c 
  src/resource_provider/storage/provider_process.hpp PRE-CREATION 


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

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


Testing
---

Test will be added later in chain.


Thanks,

Chun-Hung Hsiao

Re: Review Request 69858: Persisted intentionally dropped operations in SLRP.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 69858 was successfully built and tested.

Reviews applied: `['69858']`

All the build artifacts available at: 
http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2831/mesos-review-69858

- Mesos Reviewbot Windows


On Jan. 30, 2019, 5:38 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69858/
> ---
> 
> (Updated Jan. 30, 2019, 5:38 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gastón Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-9537
> https://issues.apache.org/jira/browse/MESOS-9537
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If an operation is dropped intentionally (e.g., because of a resource
> version mismatch), the operation should be persisted so no conflicting
> status update would be generated for operation reconciliation.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> d6e20a549ede189c757ae3ae922ab7cb86d2be2c 
>   src/resource_provider/storage/provider_process.hpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/69858/diff/2/
> 
> 
> Testing
> ---
> 
> Test will be added later in chain.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 69858: Persisted intentionally dropped operations in SLRP.

[Chun-Hung Hsiao]

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

(Updated Jan. 30, 2019, 5:38 a.m.)


Review request for mesos, Benjamin Bannier, Gastón Kleiman, and Greg Mann.


Changes
---

Added a missing checkpointing call.


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


Repository: mesos


Description
---

If an operation is dropped intentionally (e.g., because of a resource
version mismatch), the operation should be persisted so no conflicting
status update would be generated for operation reconciliation.


Diffs (updated)
-

  src/resource_provider/storage/provider.cpp 
d6e20a549ede189c757ae3ae922ab7cb86d2be2c 
  src/resource_provider/storage/provider_process.hpp PRE-CREATION 


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

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


Testing
---

Test will be added later in chain.


Thanks,

Chun-Hung Hsiao