You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jie Yu <yu...@gmail.com> on 2017/10/30 15:33:14 UTC
Review Request 63414: Added a helper to get the resoruce provider ID
from an offer operation.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63414/
-----------------------------------------------------------
Review request for mesos and Benjamin Bannier.
Bugs: MESOS-7235
https://issues.apache.org/jira/browse/MESOS-7235
Repository: mesos
Description
-------
Added a helper to get the resoruce provider ID from an offer operation.
Diffs
-----
src/common/resources_utils.hpp 18e3d9d4baad23669d00542594f5c15a989b7b9e
src/common/resources_utils.cpp e34cd8a3c9046a6f12c12a275a7b3a852b492f4c
Diff: https://reviews.apache.org/r/63414/diff/1/
Testing
-------
make check
Thanks,
Jie Yu
Re: Review Request 63414: Added a helper to get the resoruce provider
ID from an offer operation.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63414/
-----------------------------------------------------------
(Updated Nov. 1, 2017, 11:11 p.m.)
Review request for mesos and Benjamin Bannier.
Changes
-------
Addressed comments.
Bugs: MESOS-7235
https://issues.apache.org/jira/browse/MESOS-7235
Repository: mesos
Description
-------
Added a helper to get the resoruce provider ID from an offer operation.
Diffs (updated)
-----
src/common/resources_utils.hpp 18e3d9d4baad23669d00542594f5c15a989b7b9e
src/common/resources_utils.cpp e34cd8a3c9046a6f12c12a275a7b3a852b492f4c
Diff: https://reviews.apache.org/r/63414/diff/2/
Changes: https://reviews.apache.org/r/63414/diff/1-2/
Testing
-------
make check
Thanks,
Jie Yu
Re: Review Request 63414: Added a helper to get the resoruce provider
ID from an offer operation.
Posted by Jie Yu <yu...@gmail.com>.
> On Oct. 30, 2017, 3:44 p.m., Benjamin Bannier wrote:
> > src/common/resources_utils.cpp
> > Lines 104-121 (patched)
> > <https://reviews.apache.org/r/63414/diff/1/?file=1872669#file1872669line104>
> >
> > It would be great if we could explicitly `break` for any case not containing an explicit `return`.
Yeah, it's a bug. I'll add some tests.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63414/#review189597
-----------------------------------------------------------
On Oct. 30, 2017, 3:33 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63414/
> -----------------------------------------------------------
>
> (Updated Oct. 30, 2017, 3:33 p.m.)
>
>
> Review request for mesos and Benjamin Bannier.
>
>
> Bugs: MESOS-7235
> https://issues.apache.org/jira/browse/MESOS-7235
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a helper to get the resoruce provider ID from an offer operation.
>
>
> Diffs
> -----
>
> src/common/resources_utils.hpp 18e3d9d4baad23669d00542594f5c15a989b7b9e
> src/common/resources_utils.cpp e34cd8a3c9046a6f12c12a275a7b3a852b492f4c
>
>
> Diff: https://reviews.apache.org/r/63414/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 63414: Added a helper to get the resoruce provider
ID from an offer operation.
Posted by Jie Yu <yu...@gmail.com>.
> On Oct. 30, 2017, 3:44 p.m., Benjamin Bannier wrote:
> > src/common/resources_utils.cpp
> > Lines 97 (patched)
> > <https://reviews.apache.org/r/63414/diff/1/?file=1872669#file1872669line97>
> >
> > We should try to prevent the initialization and copy assignement of `resource`, e.g.,
> >
> >
> > Resource* resource = nullptr;
> >
> > switch (operation.type()) {
> > // Possibly assign to `resource`.
> > }
> >
> > CHECK_NOT_NULL(resource);
> >
> > if (resource->has_provider_id()) {
> > return resource->provider_id();
> > }
> >
> > return None();
I used `Option<Resource>` here. Using pointer is tricky because the parameter we passed in is const.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63414/#review189597
-----------------------------------------------------------
On Oct. 30, 2017, 3:33 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63414/
> -----------------------------------------------------------
>
> (Updated Oct. 30, 2017, 3:33 p.m.)
>
>
> Review request for mesos and Benjamin Bannier.
>
>
> Bugs: MESOS-7235
> https://issues.apache.org/jira/browse/MESOS-7235
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a helper to get the resoruce provider ID from an offer operation.
>
>
> Diffs
> -----
>
> src/common/resources_utils.hpp 18e3d9d4baad23669d00542594f5c15a989b7b9e
> src/common/resources_utils.cpp e34cd8a3c9046a6f12c12a275a7b3a852b492f4c
>
>
> Diff: https://reviews.apache.org/r/63414/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 63414: Added a helper to get the resoruce provider
ID from an offer operation.
Posted by Benjamin Bannier <bb...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63414/#review189597
-----------------------------------------------------------
Fix it, then Ship it!
src/common/resources_utils.hpp
Lines 51-53 (patched)
<https://reviews.apache.org/r/63414/#comment266747>
Maybe reword this, e.g.,
Therefore, the specified operation should not contain resources from more than one resource provider.
src/common/resources_utils.cpp
Lines 97 (patched)
<https://reviews.apache.org/r/63414/#comment266745>
We should try to prevent the initialization and copy assignement of `resource`, e.g.,
Resource* resource = nullptr;
switch (operation.type()) {
// Possibly assign to `resource`.
}
CHECK_NOT_NULL(resource);
if (resource->has_provider_id()) {
return resource->provider_id();
}
return None();
src/common/resources_utils.cpp
Lines 104-121 (patched)
<https://reviews.apache.org/r/63414/#comment266746>
It would be great if we could explicitly `break` for any case not containing an explicit `return`.
- Benjamin Bannier
On Oct. 30, 2017, 4:33 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63414/
> -----------------------------------------------------------
>
> (Updated Oct. 30, 2017, 4:33 p.m.)
>
>
> Review request for mesos and Benjamin Bannier.
>
>
> Bugs: MESOS-7235
> https://issues.apache.org/jira/browse/MESOS-7235
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a helper to get the resoruce provider ID from an offer operation.
>
>
> Diffs
> -----
>
> src/common/resources_utils.hpp 18e3d9d4baad23669d00542594f5c15a989b7b9e
> src/common/resources_utils.cpp e34cd8a3c9046a6f12c12a275a7b3a852b492f4c
>
>
> Diff: https://reviews.apache.org/r/63414/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 63414: Added a helper to get the resoruce provider
ID from an offer operation.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63414/#review189607
-----------------------------------------------------------
Bad patch!
Reviews applied: [63414, 63413, 63213, 63212, 63211, 63210, 63200, 63017, 59989, 59988, 59987, 60109]
Failed command: python support/apply-reviews.py -n -r 59988
Error:
2017-10-30 16:25:22 URL:https://reviews.apache.org/r/59988/diff/raw/ [1148/1148] -> "59988.patch" [1]
error: patch failed: 3rdparty/stout/tests/protobuf_tests.proto:24
error: 3rdparty/stout/tests/protobuf_tests.proto: patch does not apply
Full log: https://builds.apache.org/job/Mesos-Reviewbot/19944/console
- Mesos Reviewbot
On Oct. 30, 2017, 3:33 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63414/
> -----------------------------------------------------------
>
> (Updated Oct. 30, 2017, 3:33 p.m.)
>
>
> Review request for mesos and Benjamin Bannier.
>
>
> Bugs: MESOS-7235
> https://issues.apache.org/jira/browse/MESOS-7235
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a helper to get the resoruce provider ID from an offer operation.
>
>
> Diffs
> -----
>
> src/common/resources_utils.hpp 18e3d9d4baad23669d00542594f5c15a989b7b9e
> src/common/resources_utils.cpp e34cd8a3c9046a6f12c12a275a7b3a852b492f4c
>
>
> Diff: https://reviews.apache.org/r/63414/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 63414: Added a helper to get the resoruce provider
ID from an offer operation.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63414/#review189694
-----------------------------------------------------------
FAIL: Failed to apply the dependent review: 59988.
Failed command: `python.exe .\support\apply-reviews.py -n -r 59988`
All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/63414
Relevant logs:
- [apply-review-59988-stdout.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/63414/logs/apply-review-59988-stdout.log):
```
error: patch failed: 3rdparty/stout/tests/protobuf_tests.proto:24
error: 3rdparty/stout/tests/protobuf_tests.proto: patch does not apply
```
- Mesos Reviewbot Windows
On Oct. 30, 2017, 3:33 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63414/
> -----------------------------------------------------------
>
> (Updated Oct. 30, 2017, 3:33 p.m.)
>
>
> Review request for mesos and Benjamin Bannier.
>
>
> Bugs: MESOS-7235
> https://issues.apache.org/jira/browse/MESOS-7235
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a helper to get the resoruce provider ID from an offer operation.
>
>
> Diffs
> -----
>
> src/common/resources_utils.hpp 18e3d9d4baad23669d00542594f5c15a989b7b9e
> src/common/resources_utils.cpp e34cd8a3c9046a6f12c12a275a7b3a852b492f4c
>
>
> Diff: https://reviews.apache.org/r/63414/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>