You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Marco Massenzio <ma...@mesosphere.io> on 2015/04/23 21:36:13 UTC
Review Request 33490: MESOS-2633 Avoid adding duplicate tasks
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33490/
-----------------------------------------------------------
Review request for mesos, Adam B and Joris Van Remoortere.
Repository: mesos
Description
-------
In Framework::addCompletedTask(const Task& task) we did not check
for duplicated tasks, so they could be added more than once.
A simple check has now been added (there still is the issue
of whether the `operator ==` on `Task` is too strict, so as
never to cause a match).
Diffs
-----
src/master/framework.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/33490/diff/
Testing
-------
buildbot make check pass
http://build.mesosphere.com:5555/builders/dev_test/builds/13
Thanks,
Marco Massenzio
Re: Review Request 33490: MESOS-2633 Avoid adding duplicate tasks
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33490/#review82214
-----------------------------------------------------------
Patch looks great!
Reviews applied: [33376, 33490]
All tests passed.
- Mesos ReviewBot
On April 30, 2015, 9:46 p.m., Marco Massenzio wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33490/
> -----------------------------------------------------------
>
> (Updated April 30, 2015, 9:46 p.m.)
>
>
> Review request for mesos, Adam B and Joris Van Remoortere.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> In Framework::addCompletedTask(const Task& task) we did not check
> for duplicated tasks, so they could be added more than once.
>
> A simple check has now been added (there still is the issue
> of whether the `operator ==` on `Task` is too strict, so as
> never to cause a match).
>
>
> Diffs
> -----
>
> src/master/framework.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/33490/diff/
>
>
> Testing
> -------
>
> buildbot make check pass
> http://build.mesosphere.com:5555/builders/dev_test/builds/13
>
>
> Thanks,
>
> Marco Massenzio
>
>
Re: Review Request 33490: Avoid adding duplicate tasks
Posted by Marco Massenzio <ma...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33490/
-----------------------------------------------------------
(Updated May 8, 2015, 6:16 p.m.)
Review request for mesos, Adam B and Joris Van Remoortere.
Repository: mesos
Description (updated)
-------
In `Framework::addCompletedTask(const Task& task)` we did not check
for duplicated tasks, so they could be added more than once.
A simple check has now been added (there is the question of whether the `operator ==` on `Task` is too strict, so as
never to cause a match).
This fixes a TODO I came across as part of the work on MESOS-2633 (no Jira tracking this issue)
Diffs
-----
src/master/framework.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/33490/diff/
Testing (updated)
-------
make check
Thanks,
Marco Massenzio
Re: Review Request 33490: Avoid adding duplicate tasks
Posted by Marco Massenzio <ma...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33490/
-----------------------------------------------------------
(Updated May 8, 2015, 6:14 p.m.)
Review request for mesos, Adam B and Joris Van Remoortere.
Summary (updated)
-----------------
Avoid adding duplicate tasks
Bugs: MESOS-2633
https://issues.apache.org/jira/browse/MESOS-2633
Repository: mesos
Description
-------
In Framework::addCompletedTask(const Task& task) we did not check
for duplicated tasks, so they could be added more than once.
A simple check has now been added (there still is the issue
of whether the `operator ==` on `Task` is too strict, so as
never to cause a match).
Diffs
-----
src/master/framework.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/33490/diff/
Testing
-------
buildbot make check pass
http://build.mesosphere.com:5555/builders/dev_test/builds/13
Thanks,
Marco Massenzio
Re: Review Request 33490: Avoid adding duplicate tasks
Posted by Marco Massenzio <ma...@mesosphere.io>.
> On May 7, 2015, 6:34 p.m., Ben Mahler wrote:
> > Hm, I looked at MESOS-2633, that's only about moving implementation out?
> >
> > Could we get some more context on this change? What motivated it? Is there a bug? When can it occur? What happens if it does?
There was no Jira about this change: it was a TODO in the code, which I thought looked trivial enough that was worth doing (my general rule has always been: if it takes longer to create a ticket than actually writing the code to solve it, just do it :)
As for the reasons for the ToDo in the first place, I'll leave it up to @adam to address that - I was merely executing on his wishes...
Thanks for noting that, though: I have updated the description and elaborated on the diff description.
- Marco
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33490/#review82867
-----------------------------------------------------------
On May 8, 2015, 6:16 p.m., Marco Massenzio wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33490/
> -----------------------------------------------------------
>
> (Updated May 8, 2015, 6:16 p.m.)
>
>
> Review request for mesos, Adam B and Joris Van Remoortere.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> In `Framework::addCompletedTask(const Task& task)` we did not check
> for duplicated tasks, so they could be added more than once.
>
> A simple check has now been added (there is the question of whether the `operator ==` on `Task` is too strict, so as
> never to cause a match).
>
> This fixes a TODO I came across as part of the work on MESOS-2633 (no Jira tracking this issue)
>
>
> Diffs
> -----
>
> src/master/framework.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/33490/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Marco Massenzio
>
>
Re: Review Request 33490: MESOS-2633 Avoid adding duplicate tasks
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33490/#review82867
-----------------------------------------------------------
Hm, I looked at MESOS-2633, that's only about moving implementation out?
Could we get some more context on this change? What motivated it? Is there a bug? When can it occur? What happens if it does?
- Ben Mahler
On May 7, 2015, 8:26 a.m., Marco Massenzio wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33490/
> -----------------------------------------------------------
>
> (Updated May 7, 2015, 8:26 a.m.)
>
>
> Review request for mesos, Adam B and Joris Van Remoortere.
>
>
> Bugs: MESOS-2633
> https://issues.apache.org/jira/browse/MESOS-2633
>
>
> Repository: mesos
>
>
> Description
> -------
>
> In Framework::addCompletedTask(const Task& task) we did not check
> for duplicated tasks, so they could be added more than once.
>
> A simple check has now been added (there still is the issue
> of whether the `operator ==` on `Task` is too strict, so as
> never to cause a match).
>
>
> Diffs
> -----
>
> src/master/framework.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/33490/diff/
>
>
> Testing
> -------
>
> buildbot make check pass
> http://build.mesosphere.com:5555/builders/dev_test/builds/13
>
>
> Thanks,
>
> Marco Massenzio
>
>
Re: Review Request 33490: MESOS-2633 Avoid adding duplicate tasks
Posted by Marco Massenzio <ma...@mesosphere.io>.
> On May 7, 2015, 8:37 a.m., Adam B wrote:
> > Thanks for attacking one of my TODOs. I've got a couple of questions, but it looks pretty straightforward to me.
> >
> > I'm confused about when `==` would be too strict. What fields would you expect to differ? Maybe if you got a TASK_LOST followed by a TASK_FAILED for the same taskId, as is supposedly possible in some weird race? https://github.com/apache/mesos/blob/master/src/master/master.cpp#L3738
> >
> > Testing:
> > It's probably unfair to link to a private mesosphere-only build result in your Testing section.
> > Did the buildbot pass `make distcheck` as well as just `make check`?
> > Is there a unit test to verify your change? Do we need one?
> >
> > I have yet to learn what our recommended coding style is for lambdas now, so I'll defer to @jvanremoortere for his approval there.
This was more a question I was pondering myself: the current `==` does a "bitwise" comparison of two `Task` objects; my question was around the "semantic" meaning of equality for two `Task`s: are they *equal* if and only if they are *identical*, or is there a narrower definition of *equality* (eg, at one extreme, one could just use the `Task::task_id` and ignore all other fields).
In fact, in this case, should we just do that? (it would certainly address somewhat, if not entirely, your other concern about performance impact)
- Marco
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33490/#review82809
-----------------------------------------------------------
On May 7, 2015, 8:26 a.m., Marco Massenzio wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33490/
> -----------------------------------------------------------
>
> (Updated May 7, 2015, 8:26 a.m.)
>
>
> Review request for mesos, Adam B and Joris Van Remoortere.
>
>
> Bugs: MESOS-2633
> https://issues.apache.org/jira/browse/MESOS-2633
>
>
> Repository: mesos
>
>
> Description
> -------
>
> In Framework::addCompletedTask(const Task& task) we did not check
> for duplicated tasks, so they could be added more than once.
>
> A simple check has now been added (there still is the issue
> of whether the `operator ==` on `Task` is too strict, so as
> never to cause a match).
>
>
> Diffs
> -----
>
> src/master/framework.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/33490/diff/
>
>
> Testing
> -------
>
> buildbot make check pass
> http://build.mesosphere.com:5555/builders/dev_test/builds/13
>
>
> Thanks,
>
> Marco Massenzio
>
>
Re: Review Request 33490: MESOS-2633 Avoid adding duplicate tasks
Posted by Marco Massenzio <ma...@mesosphere.io>.
> On May 7, 2015, 8:37 a.m., Adam B wrote:
> > src/master/framework.cpp, lines 59-60
> > <https://reviews.apache.org/r/33490/diff/2/?file=946470#file946470line59>
> >
> > Given that MAX_COMPLETED_TASKS_PER_FRAMEWORK = 1000, should we worry about the performance hit of a linear scan through completedTasks each time a task completes? For very short-lived tasks, and/or frameworks that launch thousands of simultaneous tasks, this could be happening a lot.
> > Maybe completedTasks should be a combination of a circular buffer and a set.
> >
> > Taking a step further back, we've discussed replacing this circular buffer with an "Archive" abstraction, that might involve persisting the completed tasks/frameworks to disk/network, so that we can keep a longer history. If/when this happens, the duplicate checking should be pushed into the Archive::contains() implementation, rather than doing a find_if at the call-site.
> > I originally created this TODO because boost::circular_buffer doesn't have a contains().
Agreed.
Not sure how to conduct a performance to test to really verify the impact of this change, though: do you have any suggestions?
If performance is identified as an issue and we still think duplicating Tasks into the completedTask vector is an issue (is it?) we could add a hash of the Task to improve the lookup efficiency.
- Marco
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33490/#review82809
-----------------------------------------------------------
On May 7, 2015, 8:26 a.m., Marco Massenzio wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33490/
> -----------------------------------------------------------
>
> (Updated May 7, 2015, 8:26 a.m.)
>
>
> Review request for mesos, Adam B and Joris Van Remoortere.
>
>
> Bugs: MESOS-2633
> https://issues.apache.org/jira/browse/MESOS-2633
>
>
> Repository: mesos
>
>
> Description
> -------
>
> In Framework::addCompletedTask(const Task& task) we did not check
> for duplicated tasks, so they could be added more than once.
>
> A simple check has now been added (there still is the issue
> of whether the `operator ==` on `Task` is too strict, so as
> never to cause a match).
>
>
> Diffs
> -----
>
> src/master/framework.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/33490/diff/
>
>
> Testing
> -------
>
> buildbot make check pass
> http://build.mesosphere.com:5555/builders/dev_test/builds/13
>
>
> Thanks,
>
> Marco Massenzio
>
>
Re: Review Request 33490: MESOS-2633 Avoid adding duplicate tasks
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33490/#review82809
-----------------------------------------------------------
Thanks for attacking one of my TODOs. I've got a couple of questions, but it looks pretty straightforward to me.
I'm confused about when `==` would be too strict. What fields would you expect to differ? Maybe if you got a TASK_LOST followed by a TASK_FAILED for the same taskId, as is supposedly possible in some weird race? https://github.com/apache/mesos/blob/master/src/master/master.cpp#L3738
Testing:
It's probably unfair to link to a private mesosphere-only build result in your Testing section.
Did the buildbot pass `make distcheck` as well as just `make check`?
Is there a unit test to verify your change? Do we need one?
I have yet to learn what our recommended coding style is for lambdas now, so I'll defer to @jvanremoortere for his approval there.
src/master/framework.cpp
<https://reviews.apache.org/r/33490/#comment133637>
Given that MAX_COMPLETED_TASKS_PER_FRAMEWORK = 1000, should we worry about the performance hit of a linear scan through completedTasks each time a task completes? For very short-lived tasks, and/or frameworks that launch thousands of simultaneous tasks, this could be happening a lot.
Maybe completedTasks should be a combination of a circular buffer and a set.
Taking a step further back, we've discussed replacing this circular buffer with an "Archive" abstraction, that might involve persisting the completed tasks/frameworks to disk/network, so that we can keep a longer history. If/when this happens, the duplicate checking should be pushed into the Archive::contains() implementation, rather than doing a find_if at the call-site.
I originally created this TODO because boost::circular_buffer doesn't have a contains().
- Adam B
On May 7, 2015, 1:26 a.m., Marco Massenzio wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33490/
> -----------------------------------------------------------
>
> (Updated May 7, 2015, 1:26 a.m.)
>
>
> Review request for mesos, Adam B and Joris Van Remoortere.
>
>
> Bugs: MESOS-2633
> https://issues.apache.org/jira/browse/MESOS-2633
>
>
> Repository: mesos
>
>
> Description
> -------
>
> In Framework::addCompletedTask(const Task& task) we did not check
> for duplicated tasks, so they could be added more than once.
>
> A simple check has now been added (there still is the issue
> of whether the `operator ==` on `Task` is too strict, so as
> never to cause a match).
>
>
> Diffs
> -----
>
> src/master/framework.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/33490/diff/
>
>
> Testing
> -------
>
> buildbot make check pass
> http://build.mesosphere.com:5555/builders/dev_test/builds/13
>
>
> Thanks,
>
> Marco Massenzio
>
>
Re: Review Request 33490: MESOS-2633 Avoid adding duplicate tasks
Posted by Marco Massenzio <ma...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33490/
-----------------------------------------------------------
(Updated May 7, 2015, 1:26 a.m.)
Review request for mesos, Adam B and Joris Van Remoortere.
Changes
-------
Added the JIRA reference to the "Bugs" field.
Bugs: MESOS-2633
https://issues.apache.org/jira/browse/MESOS-2633
Repository: mesos
Description
-------
In Framework::addCompletedTask(const Task& task) we did not check
for duplicated tasks, so they could be added more than once.
A simple check has now been added (there still is the issue
of whether the `operator ==` on `Task` is too strict, so as
never to cause a match).
Diffs
-----
src/master/framework.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/33490/diff/
Testing
-------
buildbot make check pass
http://build.mesosphere.com:5555/builders/dev_test/builds/13
Thanks,
Marco Massenzio
Re: Review Request 33490: MESOS-2633 Avoid adding duplicate tasks
Posted by Marco Massenzio <ma...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33490/
-----------------------------------------------------------
(Updated April 30, 2015, 9:46 p.m.)
Review request for mesos, Adam B and Joris Van Remoortere.
Changes
-------
Replaced original UnaryComparator with a lambda
Repository: mesos
Description
-------
In Framework::addCompletedTask(const Task& task) we did not check
for duplicated tasks, so they could be added more than once.
A simple check has now been added (there still is the issue
of whether the `operator ==` on `Task` is too strict, so as
never to cause a match).
Diffs (updated)
-----
src/master/framework.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/33490/diff/
Testing
-------
buildbot make check pass
http://build.mesosphere.com:5555/builders/dev_test/builds/13
Thanks,
Marco Massenzio
Re: Review Request 33490: MESOS-2633 Avoid adding duplicate tasks
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33490/#review81394
-----------------------------------------------------------
Patch looks great!
Reviews applied: [33376, 33490]
All tests passed.
- Mesos ReviewBot
On April 23, 2015, 8:03 p.m., Marco Massenzio wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33490/
> -----------------------------------------------------------
>
> (Updated April 23, 2015, 8:03 p.m.)
>
>
> Review request for mesos, Adam B and Joris Van Remoortere.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> In Framework::addCompletedTask(const Task& task) we did not check
> for duplicated tasks, so they could be added more than once.
>
> A simple check has now been added (there still is the issue
> of whether the `operator ==` on `Task` is too strict, so as
> never to cause a match).
>
>
> Diffs
> -----
>
> src/master/framework.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/33490/diff/
>
>
> Testing
> -------
>
> buildbot make check pass
> http://build.mesosphere.com:5555/builders/dev_test/builds/13
>
>
> Thanks,
>
> Marco Massenzio
>
>
Re: Review Request 33490: MESOS-2633 Avoid adding duplicate tasks
Posted by Marco Massenzio <ma...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33490/
-----------------------------------------------------------
(Updated April 23, 2015, 8:03 p.m.)
Review request for mesos, Adam B and Joris Van Remoortere.
Repository: mesos
Description
-------
In Framework::addCompletedTask(const Task& task) we did not check
for duplicated tasks, so they could be added more than once.
A simple check has now been added (there still is the issue
of whether the `operator ==` on `Task` is too strict, so as
never to cause a match).
Diffs
-----
src/master/framework.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/33490/diff/
Testing
-------
buildbot make check pass
http://build.mesosphere.com:5555/builders/dev_test/builds/13
Thanks,
Marco Massenzio
Re: Review Request 33490: MESOS-2633 Avoid adding duplicate tasks
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33490/#review81379
-----------------------------------------------------------
Bad patch!
Reviews applied: [33490]
Failed command: ./support/apply-review.sh -n -r 33490
Error:
2015-04-23 19:43:56 URL:https://reviews.apache.org/r/33490/diff/raw/ [1294/1294] -> "33490.patch" [1]
error: src/master/framework.cpp: does not exist in index
Failed to apply patch
- Mesos ReviewBot
On April 23, 2015, 7:36 p.m., Marco Massenzio wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33490/
> -----------------------------------------------------------
>
> (Updated April 23, 2015, 7:36 p.m.)
>
>
> Review request for mesos, Adam B and Joris Van Remoortere.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> In Framework::addCompletedTask(const Task& task) we did not check
> for duplicated tasks, so they could be added more than once.
>
> A simple check has now been added (there still is the issue
> of whether the `operator ==` on `Task` is too strict, so as
> never to cause a match).
>
>
> Diffs
> -----
>
> src/master/framework.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/33490/diff/
>
>
> Testing
> -------
>
> buildbot make check pass
> http://build.mesosphere.com:5555/builders/dev_test/builds/13
>
>
> Thanks,
>
> Marco Massenzio
>
>