You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jacob Janco <jj...@gmail.com> on 2017/01/12 18:56:15 UTC
Re: Review Request 52534: Dispatch filter expiration twice.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52534/
-----------------------------------------------------------
(Updated Jan. 12, 2017, 6:56 p.m.)
Review request for mesos, Alexander Rukletsov, Benjamin Mahler, and Jiang Yan Xu.
Changes
-------
Change JIRA.
Bugs: MESOS-6904
https://issues.apache.org/jira/browse/MESOS-6904
Repository: mesos
Description
-------
- With an asynchronous `batch()` allocation,
this ensures that filters do not expire
before the next allocation.
- This patch should be reverted when allocation
occurs on resource recovery.
Diffs
-----
src/master/allocator/mesos/hierarchical.hpp a6424d624864155e1c87a28a63b784512c5c8722
src/master/allocator/mesos/hierarchical.cpp 91b1ec43940a788459f045ca4a4b82d4e8373bca
Diff: https://reviews.apache.org/r/52534/diff/
Testing
-------
With https://reviews.apache.org/r/51027/:
GTEST_FILTER="-*SmallOfferFilter*" make check -j8
Thanks,
Jacob Janco
Re: Review Request 52534: Dispatch filter expiration twice.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52534/#review161408
-----------------------------------------------------------
Bad patch!
Reviews applied: [52534, 51027]
Failed command: python support/apply-reviews.py -n -r 51027
Error:
Traceback (most recent call last):
File "support/apply-reviews.py", line 349, in <module>
reviewboard()
File "support/apply-reviews.py", line 328, in reviewboard
apply_review()
File "support/apply-reviews.py", line 121, in apply_review
fetch_patch()
File "support/apply-reviews.py", line 150, in fetch_patch
r = urllib2.urlopen(patch_url(), context=ssl_create_default_context())
File "support/apply-reviews.py", line 131, in ssl_create_default_context
context = ssl.SSLContext(ssl.PROTOCOL_SSLv23)
AttributeError: 'module' object has no attribute 'SSLContext'
Error in atexit._run_exitfuncs:
Traceback (most recent call last):
File "/usr/lib/python2.7/atexit.py", line 24, in _run_exitfuncs
func(*targs, **kargs)
File "support/apply-reviews.py", line 119, in <lambda>
atexit.register(lambda: os.remove('%s.patch' % patch_id()))
OSError: [Errno 2] No such file or directory: '51027.patch'
Error in sys.exitfunc:
Traceback (most recent call last):
File "/usr/lib/python2.7/atexit.py", line 24, in _run_exitfuncs
func(*targs, **kargs)
File "support/apply-reviews.py", line 119, in <lambda>
atexit.register(lambda: os.remove('%s.patch' % patch_id()))
OSError: [Errno 2] No such file or directory: '51027.patch'
Full log: https://builds.apache.org/job/Mesos-Reviewbot/16701/console
- Mesos ReviewBot
On Jan. 12, 2017, 6:56 p.m., Jacob Janco wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52534/
> -----------------------------------------------------------
>
> (Updated Jan. 12, 2017, 6:56 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Benjamin Mahler, and Jiang Yan Xu.
>
>
> Bugs: MESOS-6904
> https://issues.apache.org/jira/browse/MESOS-6904
>
>
> Repository: mesos
>
>
> Description
> -------
>
> - With an asynchronous `batch()` allocation,
> this ensures that filters do not expire
> before the next allocation.
> - This patch should be reverted when allocation
> occurs on resource recovery.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.hpp a6424d624864155e1c87a28a63b784512c5c8722
> src/master/allocator/mesos/hierarchical.cpp 91b1ec43940a788459f045ca4a4b82d4e8373bca
>
> Diff: https://reviews.apache.org/r/52534/diff/
>
>
> Testing
> -------
>
> With https://reviews.apache.org/r/51027/:
>
> GTEST_FILTER="-*SmallOfferFilter*" make check -j8
>
>
> Thanks,
>
> Jacob Janco
>
>
Re: Review Request 52534: Dispatch filter expiration twice.
Posted by Jacob Janco <jj...@gmail.com>.
> On Jan. 31, 2017, 7:39 a.m., Jiang Yan Xu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 1813-1823
> > <https://reviews.apache.org/r/52534/diff/4/?file=1619670#file1619670line1813>
> >
> > Actually no need to disambiguate now that we only have one `_expire`.
Will have to disambiguate the overloaded `expire()`
- Jacob
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52534/#review163633
-----------------------------------------------------------
On Jan. 31, 2017, 1:47 a.m., Jacob Janco wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52534/
> -----------------------------------------------------------
>
> (Updated Jan. 31, 2017, 1:47 a.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Benjamin Mahler, and Jiang Yan Xu.
>
>
> Bugs: MESOS-6904
> https://issues.apache.org/jira/browse/MESOS-6904
>
>
> Repository: mesos
>
>
> Description
> -------
>
> - With an asynchronous `batch()` allocation,
> this ensures that filters do not expire
> before the next allocation.
> - This patch should be reverted when allocation
> occurs on resource recovery.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.hpp 2cda3e311ce339d82065d68de83e86439fa192ff
> src/master/allocator/mesos/hierarchical.cpp f471b6848bebae601a7a0509e9c6ad5eab4fa4a2
>
> Diff: https://reviews.apache.org/r/52534/diff/
>
>
> Testing
> -------
>
> With https://reviews.apache.org/r/51027/:
>
> GTEST_FILTER="-*SmallOfferFilter*" make check -j8
>
>
> Thanks,
>
> Jacob Janco
>
>
Re: Review Request 52534: Dispatch filter expiration twice.
Posted by Jiang Yan Xu <xu...@apple.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52534/#review163633
-----------------------------------------------------------
Fix it, then Ship it!
src/master/allocator/mesos/hierarchical.hpp (lines 229 - 230)
<https://reviews.apache.org/r/52534/#comment235107>
This comment here seems a bit odd, we just need to describe what it does and not how it's being called.
The original comment LGTM and it can cover both `expire` and `_expire` as they are of a group.
src/master/allocator/mesos/hierarchical.hpp (line 243)
<https://reviews.apache.org/r/52534/#comment235108>
No need to change this.
src/master/allocator/mesos/hierarchical.cpp (line 932)
<https://reviews.apache.org/r/52534/#comment235109>
No need to change this.
src/master/allocator/mesos/hierarchical.cpp (lines 1061 - 1064)
<https://reviews.apache.org/r/52534/#comment235111>
The ojective is already described in the paragraph above so we don't have to reiterate it. We just need to explain the mechanism:
```
// Because `batch()` performs the allocation through a dispatch, we expire the filter also through a dispatched `_expire()` to achieve above.
```
src/master/allocator/mesos/hierarchical.cpp (lines 1806 - 1816)
<https://reviews.apache.org/r/52534/#comment235112>
Actually no need to disambiguate now that we only have one `_expire`.
- Jiang Yan Xu
On Jan. 30, 2017, 5:47 p.m., Jacob Janco wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52534/
> -----------------------------------------------------------
>
> (Updated Jan. 30, 2017, 5:47 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Benjamin Mahler, and Jiang Yan Xu.
>
>
> Bugs: MESOS-6904
> https://issues.apache.org/jira/browse/MESOS-6904
>
>
> Repository: mesos
>
>
> Description
> -------
>
> - With an asynchronous `batch()` allocation,
> this ensures that filters do not expire
> before the next allocation.
> - This patch should be reverted when allocation
> occurs on resource recovery.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.hpp 2cda3e311ce339d82065d68de83e86439fa192ff
> src/master/allocator/mesos/hierarchical.cpp f471b6848bebae601a7a0509e9c6ad5eab4fa4a2
>
> Diff: https://reviews.apache.org/r/52534/diff/
>
>
> Testing
> -------
>
> With https://reviews.apache.org/r/51027/:
>
> GTEST_FILTER="-*SmallOfferFilter*" make check -j8
>
>
> Thanks,
>
> Jacob Janco
>
>
Re: Review Request 52534: Dispatch filter expiration twice.
Posted by Jacob Janco <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52534/
-----------------------------------------------------------
(Updated Jan. 31, 2017, 9:41 p.m.)
Review request for mesos, Alexander Rukletsov, Benjamin Mahler, and Jiang Yan Xu.
Bugs: MESOS-6904
https://issues.apache.org/jira/browse/MESOS-6904
Repository: mesos
Description
-------
- With an asynchronous `batch()` allocation,
this ensures that filters do not expire
before the next allocation.
- This patch should be reverted when allocation
occurs on resource recovery.
Diffs (updated)
-----
src/master/allocator/mesos/hierarchical.hpp 2cda3e311ce339d82065d68de83e86439fa192ff
src/master/allocator/mesos/hierarchical.cpp f471b6848bebae601a7a0509e9c6ad5eab4fa4a2
Diff: https://reviews.apache.org/r/52534/diff/
Testing
-------
With https://reviews.apache.org/r/51027/:
GTEST_FILTER="-*SmallOfferFilter*" make check -j8
Thanks,
Jacob Janco
Re: Review Request 52534: Dispatch filter expiration twice.
Posted by Jacob Janco <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52534/
-----------------------------------------------------------
(Updated Jan. 31, 2017, 1:47 a.m.)
Review request for mesos, Alexander Rukletsov, Benjamin Mahler, and Jiang Yan Xu.
Changes
-------
Rebase.
Bugs: MESOS-6904
https://issues.apache.org/jira/browse/MESOS-6904
Repository: mesos
Description
-------
- With an asynchronous `batch()` allocation,
this ensures that filters do not expire
before the next allocation.
- This patch should be reverted when allocation
occurs on resource recovery.
Diffs (updated)
-----
src/master/allocator/mesos/hierarchical.hpp 2cda3e311ce339d82065d68de83e86439fa192ff
src/master/allocator/mesos/hierarchical.cpp f471b6848bebae601a7a0509e9c6ad5eab4fa4a2
Diff: https://reviews.apache.org/r/52534/diff/
Testing
-------
With https://reviews.apache.org/r/51027/:
GTEST_FILTER="-*SmallOfferFilter*" make check -j8
Thanks,
Jacob Janco
Re: Review Request 52534: Dispatch filter expiration twice.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52534/#review163233
-----------------------------------------------------------
Bad patch!
Reviews applied: [52534, 51027]
Failed command: python support/apply-reviews.py -n -r 52534
Error:
2017-01-27 06:46:11 URL:https://reviews.apache.org/r/52534/diff/raw/ [4204/4204] -> "52534.patch" [1]
error: patch failed: src/master/allocator/mesos/hierarchical.hpp:226
error: src/master/allocator/mesos/hierarchical.hpp: patch does not apply
Full log: https://builds.apache.org/job/Mesos-Reviewbot/16871/console
- Mesos Reviewbot
On Jan. 27, 2017, 2:34 a.m., Jacob Janco wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52534/
> -----------------------------------------------------------
>
> (Updated Jan. 27, 2017, 2:34 a.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Benjamin Mahler, and Jiang Yan Xu.
>
>
> Bugs: MESOS-6904
> https://issues.apache.org/jira/browse/MESOS-6904
>
>
> Repository: mesos
>
>
> Description
> -------
>
> - With an asynchronous `batch()` allocation,
> this ensures that filters do not expire
> before the next allocation.
> - This patch should be reverted when allocation
> occurs on resource recovery.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.hpp 9b66c23f26b37c02ed850c06c4518ea99078b02d
> src/master/allocator/mesos/hierarchical.cpp c2211be7458755aeb91ef078e4bfe92ac474044a
>
> Diff: https://reviews.apache.org/r/52534/diff/
>
>
> Testing
> -------
>
> With https://reviews.apache.org/r/51027/:
>
> GTEST_FILTER="-*SmallOfferFilter*" make check -j8
>
>
> Thanks,
>
> Jacob Janco
>
>
Re: Review Request 52534: Dispatch filter expiration twice.
Posted by Jacob Janco <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52534/
-----------------------------------------------------------
(Updated Jan. 27, 2017, 2:34 a.m.)
Review request for mesos, Alexander Rukletsov, Benjamin Mahler, and Jiang Yan Xu.
Bugs: MESOS-6904
https://issues.apache.org/jira/browse/MESOS-6904
Repository: mesos
Description
-------
- With an asynchronous `batch()` allocation,
this ensures that filters do not expire
before the next allocation.
- This patch should be reverted when allocation
occurs on resource recovery.
Diffs (updated)
-----
src/master/allocator/mesos/hierarchical.hpp 9b66c23f26b37c02ed850c06c4518ea99078b02d
src/master/allocator/mesos/hierarchical.cpp c2211be7458755aeb91ef078e4bfe92ac474044a
Diff: https://reviews.apache.org/r/52534/diff/
Testing
-------
With https://reviews.apache.org/r/51027/:
GTEST_FILTER="-*SmallOfferFilter*" make check -j8
Thanks,
Jacob Janco
Re: Review Request 52534: Dispatch filter expiration twice.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52534/#review162235
-----------------------------------------------------------
Fix it, then Ship it!
This adjusts the code to fix the problem, so looks good to me.
Longer term it would be great to have something that's less brittle for dealing with the timer expirations after an allocation on the agent has occurred. The current approach relies on non-local delay and dispatch knowledge, vs. an approach that was explicitly marking filters or waiting for a future (e.g. `process::collect(delay, allocationHasOccurred)`).
src/master/allocator/mesos/hierarchical.cpp (lines 1078 - 1087)
<https://reviews.apache.org/r/52534/#comment233548>
The naming here seems backwards? `_expire()` then `expire()`?
src/master/allocator/mesos/hierarchical.cpp (lines 1078 - 1081)
<https://reviews.apache.org/r/52534/#comment233549>
Hm.. seems like this should be incorporated to the comment above that is explaining when to expire the filter.
Also, maybe since the explanation is here we can just use a lambda instead of the additional top level function?
- Benjamin Mahler
On Jan. 12, 2017, 6:56 p.m., Jacob Janco wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52534/
> -----------------------------------------------------------
>
> (Updated Jan. 12, 2017, 6:56 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Benjamin Mahler, and Jiang Yan Xu.
>
>
> Bugs: MESOS-6904
> https://issues.apache.org/jira/browse/MESOS-6904
>
>
> Repository: mesos
>
>
> Description
> -------
>
> - With an asynchronous `batch()` allocation,
> this ensures that filters do not expire
> before the next allocation.
> - This patch should be reverted when allocation
> occurs on resource recovery.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.hpp a6424d624864155e1c87a28a63b784512c5c8722
> src/master/allocator/mesos/hierarchical.cpp 91b1ec43940a788459f045ca4a4b82d4e8373bca
>
> Diff: https://reviews.apache.org/r/52534/diff/
>
>
> Testing
> -------
>
> With https://reviews.apache.org/r/51027/:
>
> GTEST_FILTER="-*SmallOfferFilter*" make check -j8
>
>
> Thanks,
>
> Jacob Janco
>
>