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/03/07 13:02:57 UTC
Review Request 70154: Properly handled disk resources in operator API.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70154/
-----------------------------------------------------------
Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.
Bugs: MESOS-9637
https://issues.apache.org/jira/browse/MESOS-9637
Repository: mesos
Description
-------
This code was previously assuming that any `CREATE` operation would add
a `DiskInfo` to resources (to hold the `PersistenceInfo`). To compute
the resources required to perform the operation it then just removed the
full `DiskInfo`. This is incorrect as e.g., CSI disk resources carry
information unrelated to persistence in their `DiskInfo`.
This patch fixes `CREATE` handling by leveraging resource conversions
which were introduced in the meantime. We also refactor handlers for
other operator API calls to use the same helpers. With that less
information about the layout of operations is needed here.
Diffs
-----
src/master/http.cpp 012ee4f205ab3f2678b058fb0f3ebdac4336eca4
Diff: https://reviews.apache.org/r/70154/diff/1/
Testing
-------
`make check`
Thanks,
Benjamin Bannier
Re: Review Request 70154: Properly handled disk resources in operator
API `CREATE` handler.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70154/#review213520
-----------------------------------------------------------
Patch looks great!
Reviews applied: [70154]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers --disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On March 7, 2019, 2:41 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70154/
> -----------------------------------------------------------
>
> (Updated March 7, 2019, 2:41 p.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.
>
>
> Bugs: MESOS-9637
> https://issues.apache.org/jira/browse/MESOS-9637
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This code was previously assuming that any `CREATE` operation would add
> a `DiskInfo` to resources (to hold the `PersistenceInfo`). To compute
> the resources required to perform the operation it then just removed the
> full `DiskInfo`. This is incorrect as e.g., CSI disk resources carry
> information unrelated to persistence in their `DiskInfo`.
>
> This patch fixes `CREATE` handling by leveraging resource conversions
> which were introduced in the meantime. This allows extracting the
> required resource in a lower scope which allows us refactor handlers for
> other operator API calls. With that less information about the layout of
> operations is needed here.
>
>
> Diffs
> -----
>
> src/master/http.cpp 012ee4f205ab3f2678b058fb0f3ebdac4336eca4
> src/master/master.hpp 90e08149ece595147ca4a93da215385917a0f372
>
>
> Diff: https://reviews.apache.org/r/70154/diff/2/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 70154: Properly handled disk resources in operator
API `CREATE` handler.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70154/#review213517
-----------------------------------------------------------
Ship it!
Ship It!
- Jan Schlicht
On March 7, 2019, 2:41 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70154/
> -----------------------------------------------------------
>
> (Updated March 7, 2019, 2:41 p.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.
>
>
> Bugs: MESOS-9637
> https://issues.apache.org/jira/browse/MESOS-9637
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This code was previously assuming that any `CREATE` operation would add
> a `DiskInfo` to resources (to hold the `PersistenceInfo`). To compute
> the resources required to perform the operation it then just removed the
> full `DiskInfo`. This is incorrect as e.g., CSI disk resources carry
> information unrelated to persistence in their `DiskInfo`.
>
> This patch fixes `CREATE` handling by leveraging resource conversions
> which were introduced in the meantime. This allows extracting the
> required resource in a lower scope which allows us refactor handlers for
> other operator API calls. With that less information about the layout of
> operations is needed here.
>
>
> Diffs
> -----
>
> src/master/http.cpp 012ee4f205ab3f2678b058fb0f3ebdac4336eca4
> src/master/master.hpp 90e08149ece595147ca4a93da215385917a0f372
>
>
> Diff: https://reviews.apache.org/r/70154/diff/2/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 70154: Properly handled disk resources in operator
API `CREATE` handler.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70154/#review213532
-----------------------------------------------------------
Fix it, then Ship it!
I didn't notice until now that we don't have `src/master/http.hpp` lol.
src/master/http.cpp
Lines 3991-4003 (patched)
<https://reviews.apache.org/r/70154/#comment299483>
We can use `protobuf::getConsumedResources` instead.
- Chun-Hung Hsiao
On March 7, 2019, 1:41 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70154/
> -----------------------------------------------------------
>
> (Updated March 7, 2019, 1:41 p.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.
>
>
> Bugs: MESOS-9637
> https://issues.apache.org/jira/browse/MESOS-9637
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This code was previously assuming that any `CREATE` operation would add
> a `DiskInfo` to resources (to hold the `PersistenceInfo`). To compute
> the resources required to perform the operation it then just removed the
> full `DiskInfo`. This is incorrect as e.g., CSI disk resources carry
> information unrelated to persistence in their `DiskInfo`.
>
> This patch fixes `CREATE` handling by leveraging resource conversions
> which were introduced in the meantime. This allows extracting the
> required resource in a lower scope which allows us refactor handlers for
> other operator API calls. With that less information about the layout of
> operations is needed here.
>
>
> Diffs
> -----
>
> src/master/http.cpp 012ee4f205ab3f2678b058fb0f3ebdac4336eca4
> src/master/master.hpp 90e08149ece595147ca4a93da215385917a0f372
>
>
> Diff: https://reviews.apache.org/r/70154/diff/2/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 70154: Properly handled disk resources in operator
API `CREATE` handler.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70154/
-----------------------------------------------------------
(Updated March 7, 2019, 10:50 p.m.)
Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.
Changes
-------
Use getConsumedResources as suggested by Chun
Bugs: MESOS-9637
https://issues.apache.org/jira/browse/MESOS-9637
Repository: mesos
Description
-------
This code was previously assuming that any `CREATE` operation would add
a `DiskInfo` to resources (to hold the `PersistenceInfo`). To compute
the resources required to perform the operation it then just removed the
full `DiskInfo`. This is incorrect as e.g., CSI disk resources carry
information unrelated to persistence in their `DiskInfo`.
This patch fixes `CREATE` handling by leveraging resource conversions
which were introduced in the meantime. This allows extracting the
required resource in a lower scope which allows us refactor handlers for
other operator API calls. With that less information about the layout of
operations is needed here.
Diffs (updated)
-----
src/master/http.cpp 012ee4f205ab3f2678b058fb0f3ebdac4336eca4
src/master/master.hpp 90e08149ece595147ca4a93da215385917a0f372
Diff: https://reviews.apache.org/r/70154/diff/3/
Changes: https://reviews.apache.org/r/70154/diff/2-3/
Testing
-------
`make check`
Thanks,
Benjamin Bannier
Re: Review Request 70154: Properly handled disk resources in operator
API `CREATE` handler.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70154/
-----------------------------------------------------------
(Updated March 7, 2019, 2:41 p.m.)
Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.
Changes
-------
Push handling to a smaller scope, refactor an additional handler
Summary (updated)
-----------------
Properly handled disk resources in operator API `CREATE` handler.
Bugs: MESOS-9637
https://issues.apache.org/jira/browse/MESOS-9637
Repository: mesos
Description (updated)
-------
This code was previously assuming that any `CREATE` operation would add
a `DiskInfo` to resources (to hold the `PersistenceInfo`). To compute
the resources required to perform the operation it then just removed the
full `DiskInfo`. This is incorrect as e.g., CSI disk resources carry
information unrelated to persistence in their `DiskInfo`.
This patch fixes `CREATE` handling by leveraging resource conversions
which were introduced in the meantime. This allows extracting the
required resource in a lower scope which allows us refactor handlers for
other operator API calls. With that less information about the layout of
operations is needed here.
Diffs (updated)
-----
src/master/http.cpp 012ee4f205ab3f2678b058fb0f3ebdac4336eca4
src/master/master.hpp 90e08149ece595147ca4a93da215385917a0f372
Diff: https://reviews.apache.org/r/70154/diff/2/
Changes: https://reviews.apache.org/r/70154/diff/1-2/
Testing
-------
`make check`
Thanks,
Benjamin Bannier