You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Alexander Rukletsov <ru...@gmail.com> on 2017/02/02 09:58:30 UTC
Review Request 56218: Added some check tests for default executor.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56218/
-----------------------------------------------------------
Review request for mesos, Gast�n Kleiman and Vinod Kone.
Bugs: MESOS-6906
https://issues.apache.org/jira/browse/MESOS-6906
Repository: mesos
Description
-------
See summary.
Diffs
-----
src/tests/check_tests.cpp b5a5d8e80c80b480992a3c8160ee7d0e4443111c
Diff: https://reviews.apache.org/r/56218/diff/
Testing
-------
Tested the complete chain starting at https://reviews.apache.org/r/56208/
make check
Thanks,
Alexander Rukletsov
Re: Review Request 56218: Added some check tests for default executor.
Posted by Gastón Kleiman <ga...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56218/#review165703
-----------------------------------------------------------
Fix it, then Ship it!
Nit: the summary is a bit misleading, you are only adding one test ;-).
src/tests/check_tests.cpp (line 805)
<https://reviews.apache.org/r/56218/#comment237564>
Nit: s/http/HTTP/
src/tests/check_tests.cpp (lines 809 - 810)
<https://reviews.apache.org/r/56218/#comment237566>
We use less resources in all the tests in `default_executor_tests.cpp`:
```
Resources resources =
Resources::parse("cpus:0.1;mem:32;disk:32").get();
```
Why do we need more here? Shouldn't we be consistent with the tests in the other file?
src/tests/check_tests.cpp (line 850)
<https://reviews.apache.org/r/56218/#comment237567>
`// Ignore subsequent offers.`
src/tests/check_tests.cpp (line 902)
<https://reviews.apache.org/r/56218/#comment237568>
Vinod didn't like the `auto` in a similar test that is part of the same chain. If you follow his suggestion there, you should also follow it here. Drop this issue otherwise =).
src/tests/check_tests.cpp (line 914)
<https://reviews.apache.org/r/56218/#comment237569>
ditto
- Gast�n Kleiman
On Feb. 9, 2017, 12:56 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56218/
> -----------------------------------------------------------
>
> (Updated Feb. 9, 2017, 12:56 p.m.)
>
>
> Review request for mesos, Gast�n Kleiman and Vinod Kone.
>
>
> Bugs: MESOS-6906
> https://issues.apache.org/jira/browse/MESOS-6906
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/tests/check_tests.cpp b5a5d8e80c80b480992a3c8160ee7d0e4443111c
>
> Diff: https://reviews.apache.org/r/56218/diff/
>
>
> Testing
> -------
>
> Tested the complete chain starting at https://reviews.apache.org/r/56208/
> make check
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56218: Added some check tests for default executor.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56218/#review164941
-----------------------------------------------------------
Bad patch!
Reviews applied: [56218, 56217, 56215, 56214, 56449, 56448, 56213, 56212, 56211, 56210, 56209, 56208, 56447]
Failed command: python support/apply-reviews.py -n -r 56212
Error:
2017-02-09 17:09:30 URL:https://reviews.apache.org/r/56212/diff/raw/ [4958/4958] -> "56212.patch" [1]
error: patch failed: src/launcher/executor.cpp:778
error: src/launcher/executor.cpp: patch does not apply
Full log: https://builds.apache.org/job/Mesos-Reviewbot/17038/console
- Mesos Reviewbot
On Feb. 9, 2017, 12:56 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56218/
> -----------------------------------------------------------
>
> (Updated Feb. 9, 2017, 12:56 p.m.)
>
>
> Review request for mesos, Gast�n Kleiman and Vinod Kone.
>
>
> Bugs: MESOS-6906
> https://issues.apache.org/jira/browse/MESOS-6906
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/tests/check_tests.cpp b5a5d8e80c80b480992a3c8160ee7d0e4443111c
>
> Diff: https://reviews.apache.org/r/56218/diff/
>
>
> Testing
> -------
>
> Tested the complete chain starting at https://reviews.apache.org/r/56208/
> make check
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56218: Added a check test for default executor.
Posted by Gastón Kleiman <ga...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56218/#review169584
-----------------------------------------------------------
Ship it!
Ship It!
- Gast�n Kleiman
On March 20, 2017, 2:29 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56218/
> -----------------------------------------------------------
>
> (Updated March 20, 2017, 2:29 p.m.)
>
>
> Review request for mesos, Gast�n Kleiman and Vinod Kone.
>
>
> Bugs: MESOS-7249
> https://issues.apache.org/jira/browse/MESOS-7249
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/tests/check_tests.cpp f035c16920deaf559420ae0d7d881330ff65ae44
>
>
> Diff: https://reviews.apache.org/r/56218/diff/7/
>
>
> Testing
> -------
>
> Tested the complete chain starting at https://reviews.apache.org/r/56208/
> make check
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56218: Added a check test for default executor.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56218/
-----------------------------------------------------------
(Updated March 23, 2017, 3:30 p.m.)
Review request for mesos, Gast�n Kleiman and Vinod Kone.
Changes
-------
Disabled curl-dependent tests on Windows.
Bugs: MESOS-7249
https://issues.apache.org/jira/browse/MESOS-7249
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
src/tests/check_tests.cpp f035c16920deaf559420ae0d7d881330ff65ae44
Diff: https://reviews.apache.org/r/56218/diff/9/
Changes: https://reviews.apache.org/r/56218/diff/8-9/
Testing
-------
Tested the complete chain starting at https://reviews.apache.org/r/56208/
make check
Thanks,
Alexander Rukletsov
Re: Review Request 56218: Added a check test for default executor.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56218/
-----------------------------------------------------------
(Updated March 23, 2017, 12:12 p.m.)
Review request for mesos, Gast�n Kleiman and Vinod Kone.
Changes
-------
Rebased. NNTR.
Bugs: MESOS-7249
https://issues.apache.org/jira/browse/MESOS-7249
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
src/tests/check_tests.cpp f035c16920deaf559420ae0d7d881330ff65ae44
Diff: https://reviews.apache.org/r/56218/diff/8/
Changes: https://reviews.apache.org/r/56218/diff/7-8/
Testing
-------
Tested the complete chain starting at https://reviews.apache.org/r/56208/
make check
Thanks,
Alexander Rukletsov
Re: Review Request 56218: Added a check test for default executor.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56218/
-----------------------------------------------------------
(Updated March 20, 2017, 2:29 p.m.)
Review request for mesos, Gast�n Kleiman and Vinod Kone.
Bugs: MESOS-7249
https://issues.apache.org/jira/browse/MESOS-7249
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
src/tests/check_tests.cpp f035c16920deaf559420ae0d7d881330ff65ae44
Diff: https://reviews.apache.org/r/56218/diff/7/
Changes: https://reviews.apache.org/r/56218/diff/6-7/
Testing
-------
Tested the complete chain starting at https://reviews.apache.org/r/56208/
make check
Thanks,
Alexander Rukletsov
Re: Review Request 56218: Added a check test for default executor.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56218/
-----------------------------------------------------------
(Updated March 20, 2017, 11:57 a.m.)
Review request for mesos, Gast�n Kleiman and Vinod Kone.
Bugs: MESOS-7249
https://issues.apache.org/jira/browse/MESOS-7249
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
src/tests/check_tests.cpp f035c16920deaf559420ae0d7d881330ff65ae44
Diff: https://reviews.apache.org/r/56218/diff/6/
Changes: https://reviews.apache.org/r/56218/diff/5-6/
Testing
-------
Tested the complete chain starting at https://reviews.apache.org/r/56208/
make check
Thanks,
Alexander Rukletsov
Re: Review Request 56218: Added a check test for default executor.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On March 17, 2017, 11:07 a.m., Vinod Kone wrote:
> > src/tests/check_tests.cpp
> > Lines 854 (patched)
> > <https://reviews.apache.org/r/56218/diff/5/?file=1666170#file1666170line854>
> >
> > can you add a TODO to cleanup this file as follows:
> >
> > 1) Move variables closer to their usage
> > 2) Do not use evolve/devolve but use v1 variables/helpers wherever possible
> >
> > please take a look at default_executor_tests.cpp for an example.
I've decided not to add a TODO, but cleaned up things now.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56218/#review169278
-----------------------------------------------------------
On March 16, 2017, 4:46 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56218/
> -----------------------------------------------------------
>
> (Updated March 16, 2017, 4:46 p.m.)
>
>
> Review request for mesos, Gast�n Kleiman and Vinod Kone.
>
>
> Bugs: MESOS-7249
> https://issues.apache.org/jira/browse/MESOS-7249
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/tests/check_tests.cpp f035c16920deaf559420ae0d7d881330ff65ae44
>
>
> Diff: https://reviews.apache.org/r/56218/diff/5/
>
>
> Testing
> -------
>
> Tested the complete chain starting at https://reviews.apache.org/r/56208/
> make check
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56218: Added a check test for default executor.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On March 17, 2017, 11:07 a.m., Vinod Kone wrote:
> > src/tests/check_tests.cpp
> > Lines 846-848 (patched)
> > <https://reviews.apache.org/r/56218/diff/5/?file=1666170#file1666170line846>
> >
> > dont you want to do these tests for HTTP as well?
I think doing the same for HTTP would be redundant: shadowing, timing out is not related to the check type. However, in order to ensure these things are indeed unrealated, we should better introduce a mocked checker.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56218/#review169278
-----------------------------------------------------------
On March 16, 2017, 4:46 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56218/
> -----------------------------------------------------------
>
> (Updated March 16, 2017, 4:46 p.m.)
>
>
> Review request for mesos, Gast�n Kleiman and Vinod Kone.
>
>
> Bugs: MESOS-7249
> https://issues.apache.org/jira/browse/MESOS-7249
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/tests/check_tests.cpp f035c16920deaf559420ae0d7d881330ff65ae44
>
>
> Diff: https://reviews.apache.org/r/56218/diff/5/
>
>
> Testing
> -------
>
> Tested the complete chain starting at https://reviews.apache.org/r/56208/
> make check
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56218: Added a check test for default executor.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56218/#review169278
-----------------------------------------------------------
Fix it, then Ship it!
src/tests/check_tests.cpp
Lines 846-848 (patched)
<https://reviews.apache.org/r/56218/#comment241629>
dont you want to do these tests for HTTP as well?
src/tests/check_tests.cpp
Lines 854 (patched)
<https://reviews.apache.org/r/56218/#comment241628>
can you add a TODO to cleanup this file as follows:
1) Move variables closer to their usage
2) Do not use evolve/devolve but use v1 variables/helpers wherever possible
please take a look at default_executor_tests.cpp for an example.
- Vinod Kone
On March 16, 2017, 4:46 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56218/
> -----------------------------------------------------------
>
> (Updated March 16, 2017, 4:46 p.m.)
>
>
> Review request for mesos, Gast�n Kleiman and Vinod Kone.
>
>
> Bugs: MESOS-7249
> https://issues.apache.org/jira/browse/MESOS-7249
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/tests/check_tests.cpp f035c16920deaf559420ae0d7d881330ff65ae44
>
>
> Diff: https://reviews.apache.org/r/56218/diff/5/
>
>
> Testing
> -------
>
> Tested the complete chain starting at https://reviews.apache.org/r/56208/
> make check
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56218: Added a check test for default executor.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56218/#review169195
-----------------------------------------------------------
Bad patch!
Reviews applied: [56218, 56213, 57696, 56217, 56215, 57695, 57694, 56449]
Failed command: python support/apply-reviews.py -n -r 57695
Error:
2017-03-16 19:26:12 URL:https://reviews.apache.org/r/57695/diff/raw/ [3030/3030] -> "57695.patch" [1]
error: patch failed: src/launcher/default_executor.cpp:208
error: src/launcher/default_executor.cpp: patch does not apply
Full log: https://builds.apache.org/job/Mesos-Reviewbot/17456/console
- Mesos Reviewbot
On March 16, 2017, 9:46 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56218/
> -----------------------------------------------------------
>
> (Updated March 16, 2017, 9:46 a.m.)
>
>
> Review request for mesos, Gast�n Kleiman and Vinod Kone.
>
>
> Bugs: MESOS-7249
> https://issues.apache.org/jira/browse/MESOS-7249
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/tests/check_tests.cpp f035c16920deaf559420ae0d7d881330ff65ae44
>
>
> Diff: https://reviews.apache.org/r/56218/diff/5/
>
>
> Testing
> -------
>
> Tested the complete chain starting at https://reviews.apache.org/r/56208/
> make check
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56218: Added a check test for default executor.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56218/
-----------------------------------------------------------
(Updated March 16, 2017, 4:46 p.m.)
Review request for mesos, Gast�n Kleiman and Vinod Kone.
Summary (updated)
-----------------
Added a check test for default executor.
Bugs: MESOS-7249
https://issues.apache.org/jira/browse/MESOS-7249
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
src/tests/check_tests.cpp f035c16920deaf559420ae0d7d881330ff65ae44
Diff: https://reviews.apache.org/r/56218/diff/5/
Changes: https://reviews.apache.org/r/56218/diff/4-5/
Testing
-------
Tested the complete chain starting at https://reviews.apache.org/r/56208/
make check
Thanks,
Alexander Rukletsov
Re: Review Request 56218: Added some check tests for default executor.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56218/
-----------------------------------------------------------
(Updated Feb. 9, 2017, 12:56 p.m.)
Review request for mesos, Gast�n Kleiman and Vinod Kone.
Bugs: MESOS-6906
https://issues.apache.org/jira/browse/MESOS-6906
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
src/tests/check_tests.cpp b5a5d8e80c80b480992a3c8160ee7d0e4443111c
Diff: https://reviews.apache.org/r/56218/diff/
Testing
-------
Tested the complete chain starting at https://reviews.apache.org/r/56208/
make check
Thanks,
Alexander Rukletsov
Re: Review Request 56218: Added some check tests for default executor.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56218/#review164791
-----------------------------------------------------------
Patch looks great!
Reviews applied: [56447, 56208, 56209, 56210, 56211, 56212, 56213, 56448, 56449, 56214, 56215, 56217, 56218]
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. 8, 2017, 4:57 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56218/
> -----------------------------------------------------------
>
> (Updated Feb. 8, 2017, 4:57 p.m.)
>
>
> Review request for mesos, Gast�n Kleiman and Vinod Kone.
>
>
> Bugs: MESOS-6906
> https://issues.apache.org/jira/browse/MESOS-6906
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/tests/check_tests.cpp b5a5d8e80c80b480992a3c8160ee7d0e4443111c
>
> Diff: https://reviews.apache.org/r/56218/diff/
>
>
> Testing
> -------
>
> Tested the complete chain starting at https://reviews.apache.org/r/56208/
> make check
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56218: Added some check tests for default executor.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56218/
-----------------------------------------------------------
(Updated Feb. 8, 2017, 4:57 p.m.)
Review request for mesos, Gast�n Kleiman and Vinod Kone.
Bugs: MESOS-6906
https://issues.apache.org/jira/browse/MESOS-6906
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
src/tests/check_tests.cpp b5a5d8e80c80b480992a3c8160ee7d0e4443111c
Diff: https://reviews.apache.org/r/56218/diff/
Testing
-------
Tested the complete chain starting at https://reviews.apache.org/r/56208/
make check
Thanks,
Alexander Rukletsov
Re: Review Request 56218: Added some check tests for default executor.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56218/
-----------------------------------------------------------
(Updated Feb. 8, 2017, 3:22 p.m.)
Review request for mesos, Gast�n Kleiman and Vinod Kone.
Bugs: MESOS-6906
https://issues.apache.org/jira/browse/MESOS-6906
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
src/tests/check_tests.cpp b5a5d8e80c80b480992a3c8160ee7d0e4443111c
Diff: https://reviews.apache.org/r/56218/diff/
Testing
-------
Tested the complete chain starting at https://reviews.apache.org/r/56208/
make check
Thanks,
Alexander Rukletsov
Re: Review Request 56218: Added some check tests for default executor.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56218/#review163972
-----------------------------------------------------------
Patch looks great!
Reviews applied: [56208, 56209, 56210, 56211, 56212, 56213, 56214, 56215, 56216, 56217, 56218]
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. 2, 2017, 9:58 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56218/
> -----------------------------------------------------------
>
> (Updated Feb. 2, 2017, 9:58 a.m.)
>
>
> Review request for mesos, Gast�n Kleiman and Vinod Kone.
>
>
> Bugs: MESOS-6906
> https://issues.apache.org/jira/browse/MESOS-6906
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/tests/check_tests.cpp b5a5d8e80c80b480992a3c8160ee7d0e4443111c
>
> Diff: https://reviews.apache.org/r/56218/diff/
>
>
> Testing
> -------
>
> Tested the complete chain starting at https://reviews.apache.org/r/56208/
> make check
>
>
> Thanks,
>
> Alexander Rukletsov
>
>