You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Ben Mahler <be...@gmail.com> on 2013/10/02 02:38:58 UTC
Review Request 14438: Re-structured the Slave::tasks map.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14438/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman and Vinod Kone.
Repository: mesos-git
Description
-------
Small cleanup, no functional change here. This changes the Slave::tasks:
hashmap<std::pair<FrameworkID, TaskID>, Task*> tasks; // Before.
hashmap<FrameworkID, hashmap<TaskID, Task*> > tasks; // After.
Diffs
-----
src/master/master.hpp bd5cb1ff05967518d7dc7f3a9dc0d32c22eb9643
src/master/master.cpp a49b17ef43fca5b385a89731ca8776a26b61399a
Diff: https://reviews.apache.org/r/14438/diff/
Testing
-------
make check
Thanks,
Ben Mahler
Re: Review Request 14438: Re-structured the Slave::tasks map.
Posted by Ben Mahler <be...@gmail.com>.
> On Oct. 2, 2013, 1:10 a.m., Vinod Kone wrote:
> > src/master/master.hpp, line 314
> > <https://reviews.apache.org/r/14438/diff/1/?file=360292#file360292line314>
> >
> > what if framework_id itself is not present in the tasks map? Would this still work?
Yep, tasks[task->framework_id()] will insert and return a new empty map in this case, so the CHECK passes (and we'll always insert the framework id below). The alternative is a bit more code:
CHECK(!tasks.contains(task->framework_id()) || !tasks[task->framework_id()].contains(task->task_id())));
> On Oct. 2, 2013, 1:10 a.m., Vinod Kone wrote:
> > src/master/master.hpp, line 329
> > <https://reviews.apache.org/r/14438/diff/1/?file=360292#file360292line329>
> >
> > ditto.
In this case, if the framework id is not in the map, a new empty entry will be added and the CHECK will fail as expected. For both these cases I could update them to check that the framework id is present as well, but it's a bit more code. Any preference?
e.g.
CHECK(tasks.contains(task->framework_id()) && tasks[task->framework_id()].contains(task->task_id()));
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14438/#review26588
-----------------------------------------------------------
On Oct. 2, 2013, 12:38 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14438/
> -----------------------------------------------------------
>
> (Updated Oct. 2, 2013, 12:38 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Small cleanup, no functional change here. This changes the Slave::tasks:
>
> hashmap<std::pair<FrameworkID, TaskID>, Task*> tasks; // Before.
>
> hashmap<FrameworkID, hashmap<TaskID, Task*> > tasks; // After.
>
>
> Diffs
> -----
>
> src/master/master.hpp bd5cb1ff05967518d7dc7f3a9dc0d32c22eb9643
> src/master/master.cpp a49b17ef43fca5b385a89731ca8776a26b61399a
>
> Diff: https://reviews.apache.org/r/14438/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 14438: Re-structured the Slave::tasks map.
Posted by Vinod Kone <vi...@gmail.com>.
> On Oct. 2, 2013, 1:10 a.m., Vinod Kone wrote:
> > src/master/master.hpp, line 314
> > <https://reviews.apache.org/r/14438/diff/1/?file=360292#file360292line314>
> >
> > what if framework_id itself is not present in the tasks map? Would this still work?
>
> Ben Mahler wrote:
> Yep, tasks[task->framework_id()] will insert and return a new empty map in this case, so the CHECK passes (and we'll always insert the framework id below). The alternative is a bit more code:
>
> CHECK(!tasks.contains(task->framework_id()) || !tasks[task->framework_id()].contains(task->task_id())));
sgtm.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14438/#review26588
-----------------------------------------------------------
On Oct. 2, 2013, 5:46 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14438/
> -----------------------------------------------------------
>
> (Updated Oct. 2, 2013, 5:46 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Small cleanup, no functional change here. This changes the Slave::tasks:
>
> hashmap<std::pair<FrameworkID, TaskID>, Task*> tasks; // Before.
>
> hashmap<FrameworkID, hashmap<TaskID, Task*> > tasks; // After.
>
>
> Diffs
> -----
>
> src/master/master.hpp bd5cb1ff05967518d7dc7f3a9dc0d32c22eb9643
> src/master/master.cpp a49b17ef43fca5b385a89731ca8776a26b61399a
>
> Diff: https://reviews.apache.org/r/14438/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 14438: Re-structured the Slave::tasks map.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14438/#review26588
-----------------------------------------------------------
Ship it!
src/master/master.hpp
<https://reviews.apache.org/r/14438/#comment51833>
what if framework_id itself is not present in the tasks map? Would this still work?
src/master/master.hpp
<https://reviews.apache.org/r/14438/#comment51834>
ditto.
- Vinod Kone
On Oct. 2, 2013, 12:38 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14438/
> -----------------------------------------------------------
>
> (Updated Oct. 2, 2013, 12:38 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Small cleanup, no functional change here. This changes the Slave::tasks:
>
> hashmap<std::pair<FrameworkID, TaskID>, Task*> tasks; // Before.
>
> hashmap<FrameworkID, hashmap<TaskID, Task*> > tasks; // After.
>
>
> Diffs
> -----
>
> src/master/master.hpp bd5cb1ff05967518d7dc7f3a9dc0d32c22eb9643
> src/master/master.cpp a49b17ef43fca5b385a89731ca8776a26b61399a
>
> Diff: https://reviews.apache.org/r/14438/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 14438: Re-structured the Slave::tasks map.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14438/
-----------------------------------------------------------
(Updated Oct. 2, 2013, 5:46 a.m.)
Review request for mesos, Benjamin Hindman and Vinod Kone.
Changes
-------
Rebase.
Repository: mesos-git
Description
-------
Small cleanup, no functional change here. This changes the Slave::tasks:
hashmap<std::pair<FrameworkID, TaskID>, Task*> tasks; // Before.
hashmap<FrameworkID, hashmap<TaskID, Task*> > tasks; // After.
Diffs (updated)
-----
src/master/master.hpp bd5cb1ff05967518d7dc7f3a9dc0d32c22eb9643
src/master/master.cpp a49b17ef43fca5b385a89731ca8776a26b61399a
Diff: https://reviews.apache.org/r/14438/diff/
Testing
-------
make check
Thanks,
Ben Mahler