You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Benjamin Bannier <be...@mesosphere.io> on 2019/02/05 12:08:48 UTC
Re: Review Request 69858: Persisted intentionally dropped operations
in SLRP.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/69858/#comment298357>
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)
<https://reviews.apache.org/r/69858/#comment298358>
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)
<https://reviews.apache.org/r/69858/#comment298359>
We could introduce a variable for this.
src/resource_provider/storage/provider_process.hpp
Line 298 (original), 298-300 (patched)
<https://reviews.apache.org/r/69858/#comment298360>
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.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
> On Feb. 5, 2019, 12:08 p.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 2889 (patched)
> > <https://reviews.apache.org/r/69858/diff/3/?file=2123315#file2123315line2889>
> >
> > 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)
> > <https://reviews.apache.org/r/69858/diff/3/?file=2123315#file2123315line2898>
> >
> > 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)
> > <https://reviews.apache.org/r/69858/diff/3/?file=2123315#file2123315line2923>
> >
> > 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
>
>