You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Anand Mazumdar <an...@apache.org> on 2017/02/17 05:23:34 UTC
Review Request 56778: Fixed a bug around executor not able to use
reserved resources.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56778/
-----------------------------------------------------------
Review request for mesos and Benjamin Mahler.
Bugs: MESOS-7119
https://issues.apache.org/jira/browse/MESOS-7119
Repository: mesos
Description
-------
We were not unallocating the resources before checking if the
executor resources were contained in the checkpointed resources
on the agent.
Diffs
-----
src/slave/slave.cpp ebba8e16bc9ec45781183e78cb5a3c351a5f65f5
src/tests/default_executor_tests.cpp ffb69e9d6745c267704195056500edd4d8a4ca3f
Diff: https://reviews.apache.org/r/56778/diff/
Testing
-------
make check + added test that fails without the change.
Thanks,
Anand Mazumdar
Re: Review Request 56778: Fixed a bug around executor not able to use
reserved resources.
Posted by Anand Mazumdar <an...@apache.org>.
> On Feb. 17, 2017, 7:41 p.m., Vinod Kone wrote:
> > src/tests/default_executor_tests.cpp, lines 1416-1417
> > <https://reviews.apache.org/r/56778/diff/1/?file=1638241#file1638241line1416>
> >
> > hmm. why didn't you just start with a v1::ExecutorInfo like you did with v1::TaskGroup?
Mostly to be consistent with the other tests in the file. Would do a sweep later.
> On Feb. 17, 2017, 7:41 p.m., Vinod Kone wrote:
> > src/tests/default_executor_tests.cpp, line 1317
> > <https://reviews.apache.org/r/56778/diff/1/?file=1638241#file1638241line1317>
> >
> > Is this still needed? Thought we allowed implicit roles now?
good catch, I copy/pasted it from the reservation tests. Would fix it here and thereafter need to do a sweep there too.
> On Feb. 17, 2017, 7:41 p.m., Vinod Kone wrote:
> > src/tests/default_executor_tests.cpp, lines 1324-1335
> > <https://reviews.apache.org/r/56778/diff/1/?file=1638241#file1638241line1324>
> >
> > move this to #1380
All the other tests in this file are consistent with doing it here though. Would do a sweep.
- Anand
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56778/#review165959
-----------------------------------------------------------
On Feb. 17, 2017, 5:24 a.m., Anand Mazumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56778/
> -----------------------------------------------------------
>
> (Updated Feb. 17, 2017, 5:24 a.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-7137
> https://issues.apache.org/jira/browse/MESOS-7137
>
>
> Repository: mesos
>
>
> Description
> -------
>
> We were not unallocating the resources before checking if the
> executor resources were contained in the checkpointed resources
> on the agent.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp ebba8e16bc9ec45781183e78cb5a3c351a5f65f5
> src/tests/default_executor_tests.cpp ffb69e9d6745c267704195056500edd4d8a4ca3f
>
> Diff: https://reviews.apache.org/r/56778/diff/
>
>
> Testing
> -------
>
> make check + added test that fails without the change.
>
>
> Thanks,
>
> Anand Mazumdar
>
>
Re: Review Request 56778: Fixed a bug around executor not able to use
reserved resources.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56778/#review165959
-----------------------------------------------------------
Fix it, then Ship it!
src/tests/default_executor_tests.cpp (line 1317)
<https://reviews.apache.org/r/56778/#comment237800>
Is this still needed? Thought we allowed implicit roles now?
src/tests/default_executor_tests.cpp (lines 1324 - 1335)
<https://reviews.apache.org/r/56778/#comment237801>
move this to #1380
src/tests/default_executor_tests.cpp (lines 1416 - 1417)
<https://reviews.apache.org/r/56778/#comment237804>
hmm. why didn't you just start with a v1::ExecutorInfo like you did with v1::TaskGroup?
- Vinod Kone
On Feb. 17, 2017, 5:24 a.m., Anand Mazumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56778/
> -----------------------------------------------------------
>
> (Updated Feb. 17, 2017, 5:24 a.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-7137
> https://issues.apache.org/jira/browse/MESOS-7137
>
>
> Repository: mesos
>
>
> Description
> -------
>
> We were not unallocating the resources before checking if the
> executor resources were contained in the checkpointed resources
> on the agent.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp ebba8e16bc9ec45781183e78cb5a3c351a5f65f5
> src/tests/default_executor_tests.cpp ffb69e9d6745c267704195056500edd4d8a4ca3f
>
> Diff: https://reviews.apache.org/r/56778/diff/
>
>
> Testing
> -------
>
> make check + added test that fails without the change.
>
>
> Thanks,
>
> Anand Mazumdar
>
>
Re: Review Request 56778: Fixed a bug around executor not able to use
reserved resources.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56778/#review165910
-----------------------------------------------------------
Patch looks great!
Reviews applied: [56778]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On Feb. 17, 2017, 5:24 a.m., Anand Mazumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56778/
> -----------------------------------------------------------
>
> (Updated Feb. 17, 2017, 5:24 a.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-7137
> https://issues.apache.org/jira/browse/MESOS-7137
>
>
> Repository: mesos
>
>
> Description
> -------
>
> We were not unallocating the resources before checking if the
> executor resources were contained in the checkpointed resources
> on the agent.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp ebba8e16bc9ec45781183e78cb5a3c351a5f65f5
> src/tests/default_executor_tests.cpp ffb69e9d6745c267704195056500edd4d8a4ca3f
>
> Diff: https://reviews.apache.org/r/56778/diff/
>
>
> Testing
> -------
>
> make check + added test that fails without the change.
>
>
> Thanks,
>
> Anand Mazumdar
>
>
Re: Review Request 56778: Fixed a bug around executor not able to use
reserved resources.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56778/#review165990
-----------------------------------------------------------
@anand, can you follow up on fixing the tests?
src/tests/default_executor_tests.cpp (lines 1321 - 1326)
<https://reviews.apache.org/r/56778/#comment237864>
Consistently using v1?
src/tests/default_executor_tests.cpp (line 1388)
<https://reviews.apache.org/r/56778/#comment237861>
Ditto.
src/tests/default_executor_tests.cpp (lines 1391 - 1392)
<https://reviews.apache.org/r/56778/#comment237860>
Why we still use evolve here? I think we already support v1::createTask?
- Jie Yu
On Feb. 17, 2017, 8:15 p.m., Anand Mazumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56778/
> -----------------------------------------------------------
>
> (Updated Feb. 17, 2017, 8:15 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-7137
> https://issues.apache.org/jira/browse/MESOS-7137
>
>
> Repository: mesos
>
>
> Description
> -------
>
> We were not unallocating the resources before checking if the
> executor resources were contained in the checkpointed resources
> on the agent.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp ebba8e16bc9ec45781183e78cb5a3c351a5f65f5
> src/tests/default_executor_tests.cpp ffb69e9d6745c267704195056500edd4d8a4ca3f
>
> Diff: https://reviews.apache.org/r/56778/diff/
>
>
> Testing
> -------
>
> make check + added test that fails without the change.
>
>
> Thanks,
>
> Anand Mazumdar
>
>
Re: Review Request 56778: Fixed a bug around executor not able to use
reserved resources.
Posted by Anand Mazumdar <an...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56778/
-----------------------------------------------------------
(Updated Feb. 17, 2017, 8:15 p.m.)
Review request for mesos and Benjamin Mahler.
Changes
-------
Review comments, NNFR.
Bugs: MESOS-7137
https://issues.apache.org/jira/browse/MESOS-7137
Repository: mesos
Description
-------
We were not unallocating the resources before checking if the
executor resources were contained in the checkpointed resources
on the agent.
Diffs (updated)
-----
src/slave/slave.cpp ebba8e16bc9ec45781183e78cb5a3c351a5f65f5
src/tests/default_executor_tests.cpp ffb69e9d6745c267704195056500edd4d8a4ca3f
Diff: https://reviews.apache.org/r/56778/diff/
Testing
-------
make check + added test that fails without the change.
Thanks,
Anand Mazumdar
Re: Review Request 56778: Fixed a bug around executor not able to use
reserved resources.
Posted by Anand Mazumdar <an...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56778/
-----------------------------------------------------------
(Updated Feb. 17, 2017, 5:24 a.m.)
Review request for mesos and Benjamin Mahler.
Changes
-------
Updated bug number.
Bugs: MESOS-7137
https://issues.apache.org/jira/browse/MESOS-7137
Repository: mesos
Description
-------
We were not unallocating the resources before checking if the
executor resources were contained in the checkpointed resources
on the agent.
Diffs
-----
src/slave/slave.cpp ebba8e16bc9ec45781183e78cb5a3c351a5f65f5
src/tests/default_executor_tests.cpp ffb69e9d6745c267704195056500edd4d8a4ca3f
Diff: https://reviews.apache.org/r/56778/diff/
Testing
-------
make check + added test that fails without the change.
Thanks,
Anand Mazumdar