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 2018/07/02 22:42:16 UTC
Re: Review Request 67777: Added a helper to match agent-framework
capabilities in the allocator.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67777/#review205647
-----------------------------------------------------------
Nice :)
src/master/allocator/mesos/hierarchical.cpp
Line 1756 (original), 1755 (patched)
<https://reviews.apache.org/r/67777/#comment288535>
How about a member function of the framework struct?
```
if (!framework.isCapableOnAgent(slave)) {
...
}
```
- Benjamin Mahler
On June 28, 2018, 11 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67777/
> -----------------------------------------------------------
>
> (Updated June 28, 2018, 11 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-8916
> https://issues.apache.org/jira/browse/MESOS-8916
>
>
> Repository: mesos
>
>
> Description
> -------
>
> `isFrameworkCapableReceivingAgent` checks if a framework
> is capable of receiving resources on the agent based on
> the framework capability.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.hpp 0f6c0e96a105c64465d3f5db4ff663d8fdfe7e26
> src/master/allocator/mesos/hierarchical.cpp cbdfb2ba9c25755ac631557e0e7dbd721f861a4d
>
>
> Diff: https://reviews.apache.org/r/67777/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 67777: Added a helper to match agent-framework
capabilities in the allocator.
Posted by Meng Zhu <mz...@mesosphere.io>.
> On July 2, 2018, 3:42 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Line 1756 (original), 1755 (patched)
> > <https://reviews.apache.org/r/67777/diff/2/?file=2047194#file2047194line1756>
> >
> > How about a member function of the framework struct?
> >
> > ```
> > if (!framework.isCapableOnAgent(slave)) {
> > ...
> > }
> > ```
>
> Meng Zhu wrote:
> That would need to move nested Slave class forward or outside. Currently, all the helper functions for slave/framework are member functions of the outer class. I was trying to be consistent. If you think it is necessary, I can go ahead and overhaul the structure.
I could forward declare Slave in the outer class. Yet it would still depend on `filterGpuResources` of the outer class. Need to either cache a reference or pass in the flag. Feel cleaner this way.
- Meng
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67777/#review205647
-----------------------------------------------------------
On June 28, 2018, 4 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67777/
> -----------------------------------------------------------
>
> (Updated June 28, 2018, 4 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-8916
> https://issues.apache.org/jira/browse/MESOS-8916
>
>
> Repository: mesos
>
>
> Description
> -------
>
> `isFrameworkCapableReceivingAgent` checks if a framework
> is capable of receiving resources on the agent based on
> the framework capability.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.hpp 0f6c0e96a105c64465d3f5db4ff663d8fdfe7e26
> src/master/allocator/mesos/hierarchical.cpp cbdfb2ba9c25755ac631557e0e7dbd721f861a4d
>
>
> Diff: https://reviews.apache.org/r/67777/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 67777: Added a helper to match agent-framework
capabilities in the allocator.
Posted by Meng Zhu <mz...@mesosphere.io>.
> On July 2, 2018, 3:42 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Line 1756 (original), 1755 (patched)
> > <https://reviews.apache.org/r/67777/diff/2/?file=2047194#file2047194line1756>
> >
> > How about a member function of the framework struct?
> >
> > ```
> > if (!framework.isCapableOnAgent(slave)) {
> > ...
> > }
> > ```
That would need to move nested Slave class forward or outside. Currently, all the helper functions for slave/framework are member functions of the outer class. I was trying to be consistent. If you think it is necessary, I can go ahead and overhaul the structure.
- Meng
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67777/#review205647
-----------------------------------------------------------
On June 28, 2018, 4 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67777/
> -----------------------------------------------------------
>
> (Updated June 28, 2018, 4 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-8916
> https://issues.apache.org/jira/browse/MESOS-8916
>
>
> Repository: mesos
>
>
> Description
> -------
>
> `isFrameworkCapableReceivingAgent` checks if a framework
> is capable of receiving resources on the agent based on
> the framework capability.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.hpp 0f6c0e96a105c64465d3f5db4ff663d8fdfe7e26
> src/master/allocator/mesos/hierarchical.cpp cbdfb2ba9c25755ac631557e0e7dbd721f861a4d
>
>
> Diff: https://reviews.apache.org/r/67777/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 67777: Added a helper to match agent-framework
capabilities in the allocator.
Posted by Benjamin Mahler <bm...@apache.org>.
> On July 2, 2018, 10:42 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Line 1756 (original), 1755 (patched)
> > <https://reviews.apache.org/r/67777/diff/2/?file=2047194#file2047194line1756>
> >
> > How about a member function of the framework struct?
> >
> > ```
> > if (!framework.isCapableOnAgent(slave)) {
> > ...
> > }
> > ```
>
> Meng Zhu wrote:
> That would need to move nested Slave class forward or outside. Currently, all the helper functions for slave/framework are member functions of the outer class. I was trying to be consistent. If you think it is necessary, I can go ahead and overhaul the structure.
>
> Meng Zhu wrote:
> I could forward declare Slave in the outer class. Yet it would still depend on `filterGpuResources` of the outer class. Need to either cache a reference or pass in the flag. Feel cleaner this way.
Which functions are you referring to? I only see isRemoteSlave and isWhitelisted that look like they should be members of Slave?
Yeah I think the ideal would be to pull these structs out of being nested so that the logic can be more clearly divided between them and the allocator, like we did in master.hpp for Slave and Framework structs. We can just put the TODOs in this patch and follow up separately for that, can you mark as fixed by adding TODOs?
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67777/#review205647
-----------------------------------------------------------
On July 4, 2018, 12:19 a.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67777/
> -----------------------------------------------------------
>
> (Updated July 4, 2018, 12:19 a.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-8916
> https://issues.apache.org/jira/browse/MESOS-8916
>
>
> Repository: mesos
>
>
> Description
> -------
>
> `isFrameworkCapableReceivingAgent` checks if a framework
> is capable of receiving resources on the agent based on
> the framework capability.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.hpp 0f6c0e96a105c64465d3f5db4ff663d8fdfe7e26
> src/master/allocator/mesos/hierarchical.cpp 5a6cd3d2fc5bdbaaee2d05b9be9e83d4107c749b
>
>
> Diff: https://reviews.apache.org/r/67777/diff/3/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>