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 Mahler <bm...@apache.org> on 2019/10/09 04:43:15 UTC
Review Request 71597: Rejected invalid quota configs from registry
operation.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71597/
-----------------------------------------------------------
Review request for mesos, Andrei Sekretenko and Meng Zhu.
Bugs: MESOS-10008
https://issues.apache.org/jira/browse/MESOS-10008
Repository: mesos
Description
-------
The registry operation for updating quota was assuming that
the quota configs were valid. This validates them and fails
the operation if invalid. Note that the caller should be
validating this already, but this serves as an extra guard.
Diffs
-----
src/master/quota.cpp 4ecd3269ac092b187082675e723cb18e40cea573
Diff: https://reviews.apache.org/r/71597/diff/1/
Testing
-------
Test added in subsequent patch.
Thanks,
Benjamin Mahler
Re: Review Request 71597: Rejected invalid quota configs from registry
operation.
Posted by Benjamin Mahler <bm...@apache.org>.
> On Oct. 9, 2019, 2:17 p.m., Andrei Sekretenko wrote:
> > src/master/quota.cpp
> > Lines 72 (patched)
> > <https://reviews.apache.org/r/71597/diff/1/?file=2168791#file2168791line72>
> >
> > Shouldn't we also adjust the code which handles the failure of this registry operation?
> > https://github.com/apache/mesos/blob/43bbe365db469d5e641d71f5884bd0fb1c012ea1/src/master/quota_handler.cpp#L575
> >
> > Something like
> > `
> > CHECK(result) << "Quota registry entry mutation should never fail"
> > `
> > would produce more descriptive error in case of a bug, IMO.
Good point, added:
```
CHECK(result)
<< "An invalid quota config was supplied to the registry "
<< JSON::protobuf(configs);
```
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71597/#review218157
-----------------------------------------------------------
On Oct. 9, 2019, 4:43 a.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71597/
> -----------------------------------------------------------
>
> (Updated Oct. 9, 2019, 4:43 a.m.)
>
>
> Review request for mesos, Andrei Sekretenko and Meng Zhu.
>
>
> Bugs: MESOS-10008
> https://issues.apache.org/jira/browse/MESOS-10008
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The registry operation for updating quota was assuming that
> the quota configs were valid. This validates them and fails
> the operation if invalid. Note that the caller should be
> validating this already, but this serves as an extra guard.
>
>
> Diffs
> -----
>
> src/master/quota.cpp 4ecd3269ac092b187082675e723cb18e40cea573
>
>
> Diff: https://reviews.apache.org/r/71597/diff/1/
>
>
> Testing
> -------
>
> Test added in subsequent patch.
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 71597: Rejected invalid quota configs from registry
operation.
Posted by Andrei Sekretenko <as...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71597/#review218157
-----------------------------------------------------------
Fix it, then Ship it!
src/master/quota.cpp
Lines 72 (patched)
<https://reviews.apache.org/r/71597/#comment305727>
Shouldn't we also adjust the code which handles the failure of this registry operation?
https://github.com/apache/mesos/blob/43bbe365db469d5e641d71f5884bd0fb1c012ea1/src/master/quota_handler.cpp#L575
Something like
`
CHECK(result) << "Quota registry entry mutation should never fail"
`
would produce more descriptive error in case of a bug, IMO.
- Andrei Sekretenko
On Oct. 9, 2019, 4:43 a.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71597/
> -----------------------------------------------------------
>
> (Updated Oct. 9, 2019, 4:43 a.m.)
>
>
> Review request for mesos, Andrei Sekretenko and Meng Zhu.
>
>
> Bugs: MESOS-10008
> https://issues.apache.org/jira/browse/MESOS-10008
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The registry operation for updating quota was assuming that
> the quota configs were valid. This validates them and fails
> the operation if invalid. Note that the caller should be
> validating this already, but this serves as an extra guard.
>
>
> Diffs
> -----
>
> src/master/quota.cpp 4ecd3269ac092b187082675e723cb18e40cea573
>
>
> Diff: https://reviews.apache.org/r/71597/diff/1/
>
>
> Testing
> -------
>
> Test added in subsequent patch.
>
>
> Thanks,
>
> Benjamin Mahler
>
>