You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Jie Yu <yu...@gmail.com> on 2014/12/17 02:36:15 UTC
Review Request 29134: Added an overload of Resources::contains for
Resource object.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29134/
-----------------------------------------------------------
Review request for mesos and Ben Mahler.
Repository: mesos-git
Description
-------
See summary.
Diffs
-----
include/mesos/resources.hpp 296553a
src/common/resources.cpp 9bf7ae9
Diff: https://reviews.apache.org/r/29134/diff/
Testing
-------
make check
Thanks,
Jie Yu
Re: Review Request 29134: Added an overload of Resources::contains for
Resource object.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29134/#review65278
-----------------------------------------------------------
Patch looks great!
Reviews applied: [29134]
All tests passed.
- Mesos ReviewBot
On Dec. 17, 2014, 1:36 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29134/
> -----------------------------------------------------------
>
> (Updated Dec. 17, 2014, 1:36 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp 296553a
> src/common/resources.cpp 9bf7ae9
>
> Diff: https://reviews.apache.org/r/29134/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 29134: Added an overload of Resources::contains for
Resource object.
Posted by Jie Yu <yu...@gmail.com>.
> On Dec. 17, 2014, 1:44 a.m., Ben Mahler wrote:
> > include/mesos/resources.hpp, line 113
> > <https://reviews.apache.org/r/29134/diff/1/?file=793483#file793483line113>
> >
> > Hm.. I must be missing something subtle here. Why is this necessary to point out and differentiate with _contains?
> >
> > Is it possible for a Resources object to contain an invalid Resource?
Are you suggesting removing the NOTE?
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29134/#review65272
-----------------------------------------------------------
On Dec. 17, 2014, 1:36 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29134/
> -----------------------------------------------------------
>
> (Updated Dec. 17, 2014, 1:36 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp 296553a
> src/common/resources.cpp 9bf7ae9
>
> Diff: https://reviews.apache.org/r/29134/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 29134: Added an overload of Resources::contains for
Resource object.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29134/#review65272
-----------------------------------------------------------
include/mesos/resources.hpp
<https://reviews.apache.org/r/29134/#comment108354>
Hm.. I must be missing something subtle here. Why is this necessary to point out and differentiate with _contains?
Is it possible for a Resources object to contain an invalid Resource?
- Ben Mahler
On Dec. 17, 2014, 1:36 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29134/
> -----------------------------------------------------------
>
> (Updated Dec. 17, 2014, 1:36 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp 296553a
> src/common/resources.cpp 9bf7ae9
>
> Diff: https://reviews.apache.org/r/29134/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 29134: Added an overload of Resources::contains for
Resource object.
Posted by Jie Yu <yu...@gmail.com>.
> On Dec. 18, 2014, 12:16 a.m., Ben Mahler wrote:
> > src/common/resources.cpp, line 439
> > <https://reviews.apache.org/r/29134/diff/1/?file=793484#file793484line439>
> >
> > Can you add a NOTE here:
> >
> > // NOTE: We must validate 'that' because invalid resources can lead to false positives here, and are mesos::contains assumes resources are valid.
> > // E.g. "cpus:-1" will always return true.
Done.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29134/#review65396
-----------------------------------------------------------
On Dec. 17, 2014, 1:36 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29134/
> -----------------------------------------------------------
>
> (Updated Dec. 17, 2014, 1:36 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp 296553a
> src/common/resources.cpp 9bf7ae9
>
> Diff: https://reviews.apache.org/r/29134/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 29134: Added an overload of Resources::contains for
Resource object.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29134/#review65396
-----------------------------------------------------------
src/common/resources.cpp
<https://reviews.apache.org/r/29134/#comment108504>
Can you add a NOTE here:
// NOTE: We must validate 'that' because invalid resources can lead to false positives here, and are mesos::contains assumes resources are valid.
// E.g. "cpus:-1" will always return true.
- Ben Mahler
On Dec. 17, 2014, 1:36 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29134/
> -----------------------------------------------------------
>
> (Updated Dec. 17, 2014, 1:36 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp 296553a
> src/common/resources.cpp 9bf7ae9
>
> Diff: https://reviews.apache.org/r/29134/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 29134: Added an overload of Resources::contains for
Resource object.
Posted by Jie Yu <yu...@gmail.com>.
> On Dec. 18, 2014, 12:13 a.m., Ben Mahler wrote:
> > include/mesos/resources.hpp, line 113
> > <https://reviews.apache.org/r/29134/diff/1/?file=793483#file793483line113>
> >
> > Ok understood the issue now that we've chatted. How about we move this NOTE down to _contains and highlight that the split exists for performance reasons:
> >
> > ```
> > // Similar to 'contains(const Resource&)' but skips the validity check.
> > // This can be used to avoid the performance overhead of calling 'contains(const Resource&)' when
> > // the resource can be assumed valid (e.g. it's inside a Resources).
> > //
> > // TODO(jieyu): Measure performance overhead of validity check to ensure this is warranted.
> > bool _contains(const Resource& that) const;
> > ```
Thanks! Added.
> On Dec. 18, 2014, 12:13 a.m., Ben Mahler wrote:
> > src/common/resources.cpp, line 428
> > <https://reviews.apache.org/r/29134/diff/1/?file=793484#file793484line428>
> >
> > Mind adding a note here that we use _contains because Resources only contain valid Resource objects, and we don't want the performance hit of the validity check?
Done.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29134/#review65393
-----------------------------------------------------------
On Dec. 17, 2014, 1:36 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29134/
> -----------------------------------------------------------
>
> (Updated Dec. 17, 2014, 1:36 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp 296553a
> src/common/resources.cpp 9bf7ae9
>
> Diff: https://reviews.apache.org/r/29134/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 29134: Added an overload of Resources::contains for
Resource object.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29134/#review65393
-----------------------------------------------------------
Ship it!
include/mesos/resources.hpp
<https://reviews.apache.org/r/29134/#comment108498>
Ok understood the issue now that we've chatted. How about we move this NOTE down to _contains and highlight that the split exists for performance reasons:
```
// Similar to 'contains(const Resource&)' but skips the validity check.
// This can be used to avoid the performance overhead of calling 'contains(const Resource&)' when
// the resource can be assumed valid (e.g. it's inside a Resources).
//
// TODO(jieyu): Measure performance overhead of validity check to ensure this is warranted.
bool _contains(const Resource& that) const;
```
src/common/resources.cpp
<https://reviews.apache.org/r/29134/#comment108497>
Mind adding a note here that we use _contains because Resources only contain valid Resource objects, and we don't want the performance hit of the validity check?
- Ben Mahler
On Dec. 17, 2014, 1:36 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29134/
> -----------------------------------------------------------
>
> (Updated Dec. 17, 2014, 1:36 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp 296553a
> src/common/resources.cpp 9bf7ae9
>
> Diff: https://reviews.apache.org/r/29134/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>