You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by James Peach <jp...@apache.org> on 2017/12/30 20:06:28 UTC
Review Request 64879: Updated fetcher cache tests for the default
executor.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64879/
-----------------------------------------------------------
Review request for mesos, Alexander Rukletsov, Anand Mazumdar, and Jie Yu.
Bugs: MESOS-8366
https://issues.apache.org/jira/browse/MESOS-8366
Repository: mesos
Description
-------
Updated the task launch helper to obtain the ContainerID of the
launched task, which can be used to correctly construct the sandbox
path for both the command and default executors.
Updated the test expectations in cases where the default executor
container will invoke the fetcher more times than the test is
expecting.
Updated various test helper functions to propagate more detailed
error results that make test failure easier to debug.
Diffs
-----
src/tests/fetcher_cache_tests.cpp 7db7b7dcef27b1686ccae5a7408ff2811a3b9255
Diff: https://reviews.apache.org/r/64879/diff/1/
Testing
-------
make check (Fedora 27)
Thanks,
James Peach
Re: Review Request 64879: Updated fetcher cache tests for the default
executor.
Posted by Gaston Kleiman <ga...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64879/#review194806
-----------------------------------------------------------
Ship it!
Ship It!
- Gaston Kleiman
On Dec. 30, 2017, 12:06 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64879/
> -----------------------------------------------------------
>
> (Updated Dec. 30, 2017, 12:06 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, and Jie Yu.
>
>
> Bugs: MESOS-8366
> https://issues.apache.org/jira/browse/MESOS-8366
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Updated the task launch helper to obtain the ContainerID of the
> launched task, which can be used to correctly construct the sandbox
> path for both the command and default executors.
>
> Updated the test expectations in cases where the default executor
> container will invoke the fetcher more times than the test is
> expecting.
>
> Updated various test helper functions to propagate more detailed
> error results that make test failure easier to debug.
>
>
> Diffs
> -----
>
> src/tests/fetcher_cache_tests.cpp 7db7b7dcef27b1686ccae5a7408ff2811a3b9255
>
>
> Diff: https://reviews.apache.org/r/64879/diff/2/
>
>
> Testing
> -------
>
> make check (Fedora 27)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 64879: Updated fetcher cache tests for the default
executor.
Posted by James Peach <jp...@apache.org>.
> On Jan. 2, 2018, 7:23 p.m., Gaston Kleiman wrote:
> > src/tests/fetcher_cache_tests.cpp
> > Lines 391-393 (original), 393-399 (patched)
> > <https://reviews.apache.org/r/64879/diff/1/?file=1929288#file1929288line393>
> >
> > Can't we just do `return os::access(path, X_OK);` here?
Good point!
> On Jan. 2, 2018, 7:23 p.m., Gaston Kleiman wrote:
> > src/tests/fetcher_cache_tests.cpp
> > Line 503 (original), 523 (patched)
> > <https://reviews.apache.org/r/64879/diff/1/?file=1929288#file1929288line526>
> >
> > you're overwriting `_task.runDirectory` here, is that intentional?
Yep. I added a comment.
- James
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64879/#review194603
-----------------------------------------------------------
On Dec. 30, 2017, 8:06 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64879/
> -----------------------------------------------------------
>
> (Updated Dec. 30, 2017, 8:06 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, and Jie Yu.
>
>
> Bugs: MESOS-8366
> https://issues.apache.org/jira/browse/MESOS-8366
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Updated the task launch helper to obtain the ContainerID of the
> launched task, which can be used to correctly construct the sandbox
> path for both the command and default executors.
>
> Updated the test expectations in cases where the default executor
> container will invoke the fetcher more times than the test is
> expecting.
>
> Updated various test helper functions to propagate more detailed
> error results that make test failure easier to debug.
>
>
> Diffs
> -----
>
> src/tests/fetcher_cache_tests.cpp 7db7b7dcef27b1686ccae5a7408ff2811a3b9255
>
>
> Diff: https://reviews.apache.org/r/64879/diff/2/
>
>
> Testing
> -------
>
> make check (Fedora 27)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 64879: Updated fetcher cache tests for the default
executor.
Posted by Gaston Kleiman <ga...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64879/#review194603
-----------------------------------------------------------
src/tests/fetcher_cache_tests.cpp
Lines 391-393 (original), 393-399 (patched)
<https://reviews.apache.org/r/64879/#comment273465>
Can't we just do `return os::access(path, X_OK);` here?
src/tests/fetcher_cache_tests.cpp
Line 487 (original), 504 (patched)
<https://reviews.apache.org/r/64879/#comment273466>
I think the variable should be named `task_`:
> [...]
> prepend constructor and function arguments with a leading underscore to avoid ambiguity and / or shadowing
> [...]
> Prefer trailing underscores for use as member fields (but not required). Some trailing underscores are used to distinguish between similar variables in the same scope (think prime symbols), but this should be avoided as much as possible, including removing existing instances in the code base.
src/tests/fetcher_cache_tests.cpp
Line 503 (original), 523 (patched)
<https://reviews.apache.org/r/64879/#comment273551>
you're overwriting `_task.runDirectory` here, is that intentional?
- Gaston Kleiman
On Dec. 30, 2017, 12:06 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64879/
> -----------------------------------------------------------
>
> (Updated Dec. 30, 2017, 12:06 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, and Jie Yu.
>
>
> Bugs: MESOS-8366
> https://issues.apache.org/jira/browse/MESOS-8366
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Updated the task launch helper to obtain the ContainerID of the
> launched task, which can be used to correctly construct the sandbox
> path for both the command and default executors.
>
> Updated the test expectations in cases where the default executor
> container will invoke the fetcher more times than the test is
> expecting.
>
> Updated various test helper functions to propagate more detailed
> error results that make test failure easier to debug.
>
>
> Diffs
> -----
>
> src/tests/fetcher_cache_tests.cpp 7db7b7dcef27b1686ccae5a7408ff2811a3b9255
>
>
> Diff: https://reviews.apache.org/r/64879/diff/1/
>
>
> Testing
> -------
>
> make check (Fedora 27)
>
>
> Thanks,
>
> James Peach
>
>