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:57:52 UTC
Review Request 56213: Added check tests for command executor.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56213/
-----------------------------------------------------------
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
src/tests/mesos.hpp 71e919702bccd31bf1e14205cd1e30d473883d48
Diff: https://reviews.apache.org/r/56213/diff/
Testing
-------
See https://reviews.apache.org/r/56218/
Thanks,
Alexander Rukletsov
Re: Review Request 56213: Added check tests for command executor.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Feb. 15, 2017, 5:07 p.m., Gast�n Kleiman wrote:
> > src/tests/check_tests.cpp
> > Lines 269 (patched)
> > <https://reviews.apache.org/r/56213/diff/3/?file=1627707#file1627707line269>
> >
> > We should add:
> >
> > ```
> > ASSERT_EQ(TaskStatus::REASON_TASK_CHECK_STATUS_UPDATED, checkResult.reason());
> > ```
> >
> > We should do something like this in all tests each time we get an update.
Great suggestion, thanks!
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56213/#review165724
-----------------------------------------------------------
On Feb. 28, 2017, 3:55 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56213/
> -----------------------------------------------------------
>
> (Updated Feb. 28, 2017, 3:55 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
> src/tests/mesos.hpp b450a04dfbf3bbeeb6b605fb78097dca390cbdbe
>
>
> Diff: https://reviews.apache.org/r/56213/diff/3/
>
>
> Testing
> -------
>
> See https://reviews.apache.org/r/56218/
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56213: Added check tests for command executor.
Posted by Gastón Kleiman <ga...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56213/#review165724
-----------------------------------------------------------
src/tests/check_tests.cpp (line 269)
<https://reviews.apache.org/r/56213/#comment237576>
We should add:
```
ASSERT_EQ(TaskStatus::REASON_TASK_CHECK_STATUS_UPDATED, checkResult.reason());
```
We should do something like this in all tests each time we get an update.
- Gast�n Kleiman
On Feb. 8, 2017, 4:56 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56213/
> -----------------------------------------------------------
>
> (Updated Feb. 8, 2017, 4: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
> src/tests/mesos.hpp b450a04dfbf3bbeeb6b605fb78097dca390cbdbe
>
> Diff: https://reviews.apache.org/r/56213/diff/
>
>
> Testing
> -------
>
> See https://reviews.apache.org/r/56218/
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56213: Added check tests for command executor.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Feb. 15, 2017, 1:41 p.m., Gast�n Kleiman wrote:
> > src/tests/check_tests.cpp
> > Lines 620-625 (patched)
> > <https://reviews.apache.org/r/56213/diff/3/?file=1627707#file1627707line620>
> >
> > This has the potential of being flaky on a very busy box.
> >
> > A more robust approach could set the grace period to MAX_INT, and make the health check pass iff a file created by the check exists.
Or does not depend on which update comes first. I'll add a TODO for now, which we can fix once we actually see this test being flaky.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56213/#review165568
-----------------------------------------------------------
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/56213/
> -----------------------------------------------------------
>
> (Updated March 16, 2017, 4:46 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 f035c16920deaf559420ae0d7d881330ff65ae44
> src/tests/mesos.hpp f39e243c2c11bc1c9c757049fda2122727d1fef9
>
>
> Diff: https://reviews.apache.org/r/56213/diff/7/
>
>
> Testing
> -------
>
> See https://reviews.apache.org/r/56218/
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56213: Added check tests for command executor.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Feb. 15, 2017, 1:41 p.m., Gast�n Kleiman wrote:
> > src/tests/check_tests.cpp
> > Lines 632 (patched)
> > <https://reviews.apache.org/r/56213/diff/3/?file=1627707#file1627707line632>
> >
> > I'd add:
> >
> > ```
> > EXPECT_FALSE(updateTaskRunning->status().has_healthy());
> >
> > EXPECT_TRUE(updateTaskRunning->status().has_check_status());
> > EXPECT_TRUE(updateTaskRunning->status().check_status().has_command());
> > EXPECT_FALSE(updateTaskRunning->status().check_status().command().has_exit_code());
> > ```
I'd rather not. I would like to check that one does not shadow the other, hence
1) we probably don't want to make assumptions about how inital health is delivered here (btw, this is something we need to change)
2) repeat checking initial check status (done in `CommandCheckDeliveredAndReconciled`).
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56213/#review165568
-----------------------------------------------------------
On Feb. 28, 2017, 3:55 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56213/
> -----------------------------------------------------------
>
> (Updated Feb. 28, 2017, 3:55 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
> src/tests/mesos.hpp b450a04dfbf3bbeeb6b605fb78097dca390cbdbe
>
>
> Diff: https://reviews.apache.org/r/56213/diff/3/
>
>
> Testing
> -------
>
> See https://reviews.apache.org/r/56218/
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56213: Added check tests for command executor.
Posted by Gastón Kleiman <ga...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56213/#review165568
-----------------------------------------------------------
src/tests/check_tests.cpp (line 202)
<https://reviews.apache.org/r/56213/#comment237423>
Add here and in the other tests:
`// Ignore subsequent offers.`
src/tests/check_tests.cpp (lines 620 - 625)
<https://reviews.apache.org/r/56213/#comment237550>
This has the potential of being flaky on a very busy box.
A more robust approach could set the grace period to MAX_INT, and make the health check pass iff a file created by the check exists.
src/tests/check_tests.cpp (line 632)
<https://reviews.apache.org/r/56213/#comment237553>
I'd add:
```
EXPECT_FALSE(updateTaskRunning->status().has_healthy());
EXPECT_TRUE(updateTaskRunning->status().has_check_status());
EXPECT_TRUE(updateTaskRunning->status().check_status().has_command());
EXPECT_FALSE(updateTaskRunning->status().check_status().command().has_exit_code());
```
src/tests/check_tests.cpp (line 681)
<https://reviews.apache.org/r/56213/#comment237554>
s/http/HTTP/
- Gast�n Kleiman
On Feb. 8, 2017, 4:56 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56213/
> -----------------------------------------------------------
>
> (Updated Feb. 8, 2017, 4: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
> src/tests/mesos.hpp b450a04dfbf3bbeeb6b605fb78097dca390cbdbe
>
> Diff: https://reviews.apache.org/r/56213/diff/
>
>
> Testing
> -------
>
> See https://reviews.apache.org/r/56218/
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56213: Added check tests for command executor.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Feb. 10, 2017, 2:42 a.m., Vinod Kone wrote:
> > src/tests/check_tests.cpp
> > Lines 73 (patched)
> > <https://reviews.apache.org/r/56213/diff/3/?file=1627707#file1627707line73>
> >
> > Any particular reason for using v1 scheduler API? The general recommendation has been to use v0 scheduler API unless you are testing v1 features. The main reason being most users are still using v0 scheduler API.
>
> Alexander Rukletsov wrote:
> My original intention was to use unversioned HTTP API. However, I didn't figure out how to do this and I suspect that it is not even possible. Is it correct, that there are two choices?
> 1) use older driver-based API
> 2) use newer v1 HTTP API
>
> Is the recommendation then to use older driver-based API in tests?
>
> Vinod Kone wrote:
> yes, the recommendation is to use v0 scheduler API unless either the rest of the tests in the file are all using v1 scheduler API or you are testing v1 scheduler API functionality.
In this file, we will be testing default executor as well, which only supports v1. Hence, for consistency, I decided to use V1 for all the tests in the file.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56213/#review165049
-----------------------------------------------------------
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/56213/
> -----------------------------------------------------------
>
> (Updated March 16, 2017, 4:46 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 f035c16920deaf559420ae0d7d881330ff65ae44
> src/tests/mesos.hpp f39e243c2c11bc1c9c757049fda2122727d1fef9
>
>
> Diff: https://reviews.apache.org/r/56213/diff/7/
>
>
> Testing
> -------
>
> See https://reviews.apache.org/r/56218/
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56213: Added check tests for command executor.
Posted by Gastón Kleiman <ga...@mesosphere.io>.
> On Feb. 10, 2017, 2:42 a.m., Vinod Kone wrote:
> > src/tests/check_tests.cpp, line 177
> > <https://reviews.apache.org/r/56213/diff/3/?file=1627707#file1627707line177>
> >
> > s/agent/slave/
> >
> > here and everywhere else.
> >
> > we are not doing this change yet.
>
> Gast�n Kleiman wrote:
> We do this in `health_check_tests.cpp`:
>
> ```
> $ grep "agent = " health_check_tests.cpp | wc -l
> 15
> $ grep "slave = " health_check_tests.cpp | wc -l
> 0
> $
> ```
>
> Vinod Kone wrote:
> I think we should fix `health_check_tests.cpp` as well then.
I used `health_check_tests.cpp` just as an example, but we're using this in a lot of places:
```
$ git grep "agent[^ ]* =" | cut -d: -f1 | sort | uniq
api_tests.cpp
authentication_tests.cpp
containerizer/cgroups_isolator_tests.cpp
containerizer/docker_containerizer_tests.cpp
containerizer/io_switchboard_tests.cpp
containerizer/memory_pressure_tests.cpp
fault_tolerance_tests.cpp
health_check_tests.cpp
hierarchical_allocator_tests.cpp
hook_tests.cpp
master_allocator_tests.cpp
master_tests.cpp
master_validation_tests.cpp
mesos.cpp
mesos.hpp
metrics_tests.cpp
oversubscription_tests.cpp
partition_tests.cpp
reconciliation_tests.cpp
reservation_endpoints_tests.cpp
reservation_tests.cpp
slave_authorization_tests.cpp
slave_recovery_tests.cpp
slave_tests.cpp
sorter_tests.cpp
$ git grep "agent[^ ]* =" | wc -l
207
$
```
- Gast�n
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56213/#review165049
-----------------------------------------------------------
On Feb. 8, 2017, 4:56 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56213/
> -----------------------------------------------------------
>
> (Updated Feb. 8, 2017, 4: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
> src/tests/mesos.hpp b450a04dfbf3bbeeb6b605fb78097dca390cbdbe
>
> Diff: https://reviews.apache.org/r/56213/diff/
>
>
> Testing
> -------
>
> See https://reviews.apache.org/r/56218/
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56213: Added check tests for command executor.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Feb. 10, 2017, 2:42 a.m., Vinod Kone wrote:
> > src/tests/check_tests.cpp
> > Lines 62-63 (patched)
> > <https://reviews.apache.org/r/56213/diff/3/?file=1627707#file1627707line62>
> >
> > i don't see tests for these in this review. if they are added in the latter part of the chain, please don't mention them here. or add a TODO in front of them.
Moved them to the next review.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56213/#review165049
-----------------------------------------------------------
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/56213/
> -----------------------------------------------------------
>
> (Updated March 16, 2017, 4:46 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 f035c16920deaf559420ae0d7d881330ff65ae44
> src/tests/mesos.hpp f39e243c2c11bc1c9c757049fda2122727d1fef9
>
>
> Diff: https://reviews.apache.org/r/56213/diff/7/
>
>
> Testing
> -------
>
> See https://reviews.apache.org/r/56218/
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56213: Added check tests for command executor.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Feb. 10, 2017, 2:42 a.m., Vinod Kone wrote:
> > src/tests/check_tests.cpp
> > Lines 177 (patched)
> > <https://reviews.apache.org/r/56213/diff/3/?file=1627707#file1627707line177>
> >
> > s/agent/slave/
> >
> > here and everywhere else.
> >
> > we are not doing this change yet.
>
> Gast�n Kleiman wrote:
> We do this in `health_check_tests.cpp`:
>
> ```
> $ grep "agent = " health_check_tests.cpp | wc -l
> 15
> $ grep "slave = " health_check_tests.cpp | wc -l
> 0
> $
> ```
>
> Vinod Kone wrote:
> I think we should fix `health_check_tests.cpp` as well then.
>
> Gast�n Kleiman wrote:
> I used `health_check_tests.cpp` just as an example, but we're using this in a lot of places:
>
> ```
> $ git grep "agent[^ ]* =" | cut -d: -f1 | sort | uniq
> api_tests.cpp
> authentication_tests.cpp
> containerizer/cgroups_isolator_tests.cpp
> containerizer/docker_containerizer_tests.cpp
> containerizer/io_switchboard_tests.cpp
> containerizer/memory_pressure_tests.cpp
> fault_tolerance_tests.cpp
> health_check_tests.cpp
> hierarchical_allocator_tests.cpp
> hook_tests.cpp
> master_allocator_tests.cpp
> master_tests.cpp
> master_validation_tests.cpp
> mesos.cpp
> mesos.hpp
> metrics_tests.cpp
> oversubscription_tests.cpp
> partition_tests.cpp
> reconciliation_tests.cpp
> reservation_endpoints_tests.cpp
> reservation_tests.cpp
> slave_authorization_tests.cpp
> slave_recovery_tests.cpp
> slave_tests.cpp
> sorter_tests.cpp
> $ git grep "agent[^ ]* =" | wc -l
> 207
> $
> ```
>
> Alexander Rukletsov wrote:
> The policy I'm sticking to is to use `agent` in all new or being modified non-user facing code. I'd suggest we keep `agent` here and update in other places as we go.
>
> Vinod Kone wrote:
> I spot checked some of these files, and looks like the code has been updated recently to rename `slaveFlags` to `agentFlags` (which I suspect is what the grep above is capturing) which is unfortunate. I think this arose from a single commit that Alex Clemmer did.
>
> It's my fault to not clearly articulate on dev list how phase 2 of slave to agent rename should be done. But the policy that I've been suggesting to my reviwees, is to not make a file locally inconsistent. IOW, if the rest of the file is calling it `agent` use `agent`, otherwise stick to `slave`. I think this applies to any such change not just slave to agent rename. The reason being, a new dev coming to a file and adding a new test, would be very confused if a file has a mix of old and new style.
>
> So, if you really want to use `agent` here, I recommend to do a sweep to update this file to do it across all tests (in a dependent review of course).
>
> Does that make sense?
This is a new file and it only uses `agent*` for variable names. Hence I suggest we keep it as is without renaming everything back to `slave*`.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56213/#review165049
-----------------------------------------------------------
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/56213/
> -----------------------------------------------------------
>
> (Updated March 16, 2017, 4:46 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 f035c16920deaf559420ae0d7d881330ff65ae44
> src/tests/mesos.hpp f39e243c2c11bc1c9c757049fda2122727d1fef9
>
>
> Diff: https://reviews.apache.org/r/56213/diff/7/
>
>
> Testing
> -------
>
> See https://reviews.apache.org/r/56218/
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56213: Added check tests for command executor.
Posted by Gastón Kleiman <ga...@mesosphere.io>.
> On Feb. 10, 2017, 2:42 a.m., Vinod Kone wrote:
> > src/tests/check_tests.cpp, line 177
> > <https://reviews.apache.org/r/56213/diff/3/?file=1627707#file1627707line177>
> >
> > s/agent/slave/
> >
> > here and everywhere else.
> >
> > we are not doing this change yet.
We do this in `health_check_tests.cpp`:
```
$ grep "agent = " health_check_tests.cpp | wc -l
15
$ grep "slave = " health_check_tests.cpp | wc -l
0
$
```
- Gast�n
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56213/#review165049
-----------------------------------------------------------
On Feb. 8, 2017, 4:56 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56213/
> -----------------------------------------------------------
>
> (Updated Feb. 8, 2017, 4: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
> src/tests/mesos.hpp b450a04dfbf3bbeeb6b605fb78097dca390cbdbe
>
> Diff: https://reviews.apache.org/r/56213/diff/
>
>
> Testing
> -------
>
> See https://reviews.apache.org/r/56218/
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56213: Added check tests for command executor.
Posted by Vinod Kone <vi...@gmail.com>.
> On Feb. 10, 2017, 2:42 a.m., Vinod Kone wrote:
> > src/tests/check_tests.cpp
> > Lines 73 (patched)
> > <https://reviews.apache.org/r/56213/diff/3/?file=1627707#file1627707line73>
> >
> > Any particular reason for using v1 scheduler API? The general recommendation has been to use v0 scheduler API unless you are testing v1 features. The main reason being most users are still using v0 scheduler API.
>
> Alexander Rukletsov wrote:
> My original intention was to use unversioned HTTP API. However, I didn't figure out how to do this and I suspect that it is not even possible. Is it correct, that there are two choices?
> 1) use older driver-based API
> 2) use newer v1 HTTP API
>
> Is the recommendation then to use older driver-based API in tests?
yes, the recommendation is to use v0 scheduler API unless either the rest of the tests in the file are all using v1 scheduler API or you are testing v1 scheduler API functionality.
> On Feb. 10, 2017, 2:42 a.m., Vinod Kone wrote:
> > src/tests/check_tests.cpp
> > Lines 177 (patched)
> > <https://reviews.apache.org/r/56213/diff/3/?file=1627707#file1627707line177>
> >
> > s/agent/slave/
> >
> > here and everywhere else.
> >
> > we are not doing this change yet.
>
> Gast�n Kleiman wrote:
> We do this in `health_check_tests.cpp`:
>
> ```
> $ grep "agent = " health_check_tests.cpp | wc -l
> 15
> $ grep "slave = " health_check_tests.cpp | wc -l
> 0
> $
> ```
>
> Vinod Kone wrote:
> I think we should fix `health_check_tests.cpp` as well then.
>
> Gast�n Kleiman wrote:
> I used `health_check_tests.cpp` just as an example, but we're using this in a lot of places:
>
> ```
> $ git grep "agent[^ ]* =" | cut -d: -f1 | sort | uniq
> api_tests.cpp
> authentication_tests.cpp
> containerizer/cgroups_isolator_tests.cpp
> containerizer/docker_containerizer_tests.cpp
> containerizer/io_switchboard_tests.cpp
> containerizer/memory_pressure_tests.cpp
> fault_tolerance_tests.cpp
> health_check_tests.cpp
> hierarchical_allocator_tests.cpp
> hook_tests.cpp
> master_allocator_tests.cpp
> master_tests.cpp
> master_validation_tests.cpp
> mesos.cpp
> mesos.hpp
> metrics_tests.cpp
> oversubscription_tests.cpp
> partition_tests.cpp
> reconciliation_tests.cpp
> reservation_endpoints_tests.cpp
> reservation_tests.cpp
> slave_authorization_tests.cpp
> slave_recovery_tests.cpp
> slave_tests.cpp
> sorter_tests.cpp
> $ git grep "agent[^ ]* =" | wc -l
> 207
> $
> ```
>
> Alexander Rukletsov wrote:
> The policy I'm sticking to is to use `agent` in all new or being modified non-user facing code. I'd suggest we keep `agent` here and update in other places as we go.
I spot checked some of these files, and looks like the code has been updated recently to rename `slaveFlags` to `agentFlags` (which I suspect is what the grep above is capturing) which is unfortunate. I think this arose from a single commit that Alex Clemmer did.
It's my fault to not clearly articulate on dev list how phase 2 of slave to agent rename should be done. But the policy that I've been suggesting to my reviwees, is to not make a file locally inconsistent. IOW, if the rest of the file is calling it `agent` use `agent`, otherwise stick to `slave`. I think this applies to any such change not just slave to agent rename. The reason being, a new dev coming to a file and adding a new test, would be very confused if a file has a mix of old and new style.
So, if you really want to use `agent` here, I recommend to do a sweep to update this file to do it across all tests (in a dependent review of course).
Does that make sense?
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56213/#review165049
-----------------------------------------------------------
On March 1, 2017, 12:50 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56213/
> -----------------------------------------------------------
>
> (Updated March 1, 2017, 12:50 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 f035c16920deaf559420ae0d7d881330ff65ae44
> src/tests/mesos.hpp b450a04dfbf3bbeeb6b605fb78097dca390cbdbe
>
>
> Diff: https://reviews.apache.org/r/56213/diff/4/
>
>
> Testing
> -------
>
> See https://reviews.apache.org/r/56218/
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56213: Added check tests for command executor.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Feb. 10, 2017, 2:42 a.m., Vinod Kone wrote:
> > src/tests/check_tests.cpp
> > Lines 73 (patched)
> > <https://reviews.apache.org/r/56213/diff/3/?file=1627707#file1627707line73>
> >
> > Any particular reason for using v1 scheduler API? The general recommendation has been to use v0 scheduler API unless you are testing v1 features. The main reason being most users are still using v0 scheduler API.
My original intention was to use unversioned HTTP API. However, I didn't figure out how to do this and I suspect that it is not even possible. Is it correct, that there are two choices?
1) use older driver-based API
2) use newer v1 HTTP API
Is the recommendation then to use older driver-based API in tests?
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56213/#review165049
-----------------------------------------------------------
On Feb. 28, 2017, 3:55 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56213/
> -----------------------------------------------------------
>
> (Updated Feb. 28, 2017, 3:55 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
> src/tests/mesos.hpp b450a04dfbf3bbeeb6b605fb78097dca390cbdbe
>
>
> Diff: https://reviews.apache.org/r/56213/diff/3/
>
>
> Testing
> -------
>
> See https://reviews.apache.org/r/56218/
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56213: Added check tests for command executor.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Feb. 10, 2017, 2:42 a.m., Vinod Kone wrote:
> > src/tests/check_tests.cpp
> > Lines 138 (patched)
> > <https://reviews.apache.org/r/56213/diff/3/?file=1627707#file1627707line138>
> >
> > no need for this.
For implicit reconciliation, `tasks` are empty, hence we need to ensure the `Call` message is properly formed.
> On Feb. 10, 2017, 2:42 a.m., Vinod Kone wrote:
> > src/tests/check_tests.cpp
> > Lines 171 (patched)
> > <https://reviews.apache.org/r/56213/diff/3/?file=1627707#file1627707line171>
> >
> > Instead of these long test names how about you create a fixture for CommandExecutor check tests for better namespacing?
> >
> > class CommandExecutorCheckTest : public CheckTest {};
> >
> > TEST_F(CommandExecutorCheckTest, CommandCheckDeliveredAndReconciled)
Good idea!
> On Feb. 10, 2017, 2:42 a.m., Vinod Kone wrote:
> > src/tests/check_tests.cpp
> > Lines 177 (patched)
> > <https://reviews.apache.org/r/56213/diff/3/?file=1627707#file1627707line177>
> >
> > s/agent/slave/
> >
> > here and everywhere else.
> >
> > we are not doing this change yet.
>
> Gast�n Kleiman wrote:
> We do this in `health_check_tests.cpp`:
>
> ```
> $ grep "agent = " health_check_tests.cpp | wc -l
> 15
> $ grep "slave = " health_check_tests.cpp | wc -l
> 0
> $
> ```
>
> Vinod Kone wrote:
> I think we should fix `health_check_tests.cpp` as well then.
>
> Gast�n Kleiman wrote:
> I used `health_check_tests.cpp` just as an example, but we're using this in a lot of places:
>
> ```
> $ git grep "agent[^ ]* =" | cut -d: -f1 | sort | uniq
> api_tests.cpp
> authentication_tests.cpp
> containerizer/cgroups_isolator_tests.cpp
> containerizer/docker_containerizer_tests.cpp
> containerizer/io_switchboard_tests.cpp
> containerizer/memory_pressure_tests.cpp
> fault_tolerance_tests.cpp
> health_check_tests.cpp
> hierarchical_allocator_tests.cpp
> hook_tests.cpp
> master_allocator_tests.cpp
> master_tests.cpp
> master_validation_tests.cpp
> mesos.cpp
> mesos.hpp
> metrics_tests.cpp
> oversubscription_tests.cpp
> partition_tests.cpp
> reconciliation_tests.cpp
> reservation_endpoints_tests.cpp
> reservation_tests.cpp
> slave_authorization_tests.cpp
> slave_recovery_tests.cpp
> slave_tests.cpp
> sorter_tests.cpp
> $ git grep "agent[^ ]* =" | wc -l
> 207
> $
> ```
The policy I'm sticking to is to use `agent` in all new or being modified non-user facing code. I'd suggest we keep `agent` here and update in other places as we go.
> On Feb. 10, 2017, 2:42 a.m., Vinod Kone wrote:
> > src/tests/check_tests.cpp
> > Lines 237 (patched)
> > <https://reviews.apache.org/r/56213/diff/3/?file=1627707#file1627707line237>
> >
> > s/checkCommand/command/
> >
> > here and below.
We use `command` for task's command, e.g., in `HTTPCheckDelivered`, hence I'd prefer to keep `checkCommand` to disambiguate.
> On Feb. 10, 2017, 2:42 a.m., Vinod Kone wrote:
> > src/tests/check_tests.cpp
> > Lines 391 (patched)
> > <https://reviews.apache.org/r/56213/diff/3/?file=1627707#file1627707line391>
> >
> > s/checkCommandString/command/
To avoid the aforementioned ambiguity, I'd go with `checkCommand`.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56213/#review165049
-----------------------------------------------------------
On Feb. 28, 2017, 3:55 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56213/
> -----------------------------------------------------------
>
> (Updated Feb. 28, 2017, 3:55 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
> src/tests/mesos.hpp b450a04dfbf3bbeeb6b605fb78097dca390cbdbe
>
>
> Diff: https://reviews.apache.org/r/56213/diff/3/
>
>
> Testing
> -------
>
> See https://reviews.apache.org/r/56218/
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56213: Added check tests for command executor.
Posted by Vinod Kone <vi...@gmail.com>.
> On Feb. 10, 2017, 2:42 a.m., Vinod Kone wrote:
> > src/tests/check_tests.cpp, line 177
> > <https://reviews.apache.org/r/56213/diff/3/?file=1627707#file1627707line177>
> >
> > s/agent/slave/
> >
> > here and everywhere else.
> >
> > we are not doing this change yet.
>
> Gast�n Kleiman wrote:
> We do this in `health_check_tests.cpp`:
>
> ```
> $ grep "agent = " health_check_tests.cpp | wc -l
> 15
> $ grep "slave = " health_check_tests.cpp | wc -l
> 0
> $
> ```
I think we should fix `health_check_tests.cpp` as well then.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56213/#review165049
-----------------------------------------------------------
On Feb. 8, 2017, 4:56 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56213/
> -----------------------------------------------------------
>
> (Updated Feb. 8, 2017, 4: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
> src/tests/mesos.hpp b450a04dfbf3bbeeb6b605fb78097dca390cbdbe
>
> Diff: https://reviews.apache.org/r/56213/diff/
>
>
> Testing
> -------
>
> See https://reviews.apache.org/r/56218/
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56213: Added check tests for command executor.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56213/#review165049
-----------------------------------------------------------
src/tests/check_tests.cpp (lines 62 - 63)
<https://reviews.apache.org/r/56213/#comment236889>
i don't see tests for these in this review. if they are added in the latter part of the chain, please don't mention them here. or add a TODO in front of them.
src/tests/check_tests.cpp (line 68)
<https://reviews.apache.org/r/56213/#comment236886>
Remove `call` prefixes for these functions.
src/tests/check_tests.cpp (line 73)
<https://reviews.apache.org/r/56213/#comment236905>
Any particular reason for using v1 scheduler API? The general recommendation has been to use v0 scheduler API unless you are testing v1 features. The main reason being most users are still using v0 scheduler API.
src/tests/check_tests.cpp (line 138)
<https://reviews.apache.org/r/56213/#comment236887>
no need for this.
src/tests/check_tests.cpp (line 141)
<https://reviews.apache.org/r/56213/#comment236888>
s/reconciledTask/reconcile/
src/tests/check_tests.cpp (line 171)
<https://reviews.apache.org/r/56213/#comment236890>
Instead of these long test names how about you create a fixture for CommandExecutor check tests for better namespacing?
class CommandExecutorCheckTest : public CheckTest {};
TEST_F(CommandExecutorCheckTest, CommandCheckDeliveredAndReconciled)
src/tests/check_tests.cpp (line 177)
<https://reviews.apache.org/r/56213/#comment236893>
s/agent/slave/
here and everywhere else.
we are not doing this change yet.
src/tests/check_tests.cpp (line 237)
<https://reviews.apache.org/r/56213/#comment236894>
s/checkCommand/command/
here and below.
src/tests/check_tests.cpp (line 241)
<https://reviews.apache.org/r/56213/#comment236895>
s/exitStatusVar/variable/
here and below.
src/tests/check_tests.cpp (line 249)
<https://reviews.apache.org/r/56213/#comment236892>
why auto here? it's not clear to me what the type is from just looking at this statement.
please fix it here and everywhere else.
src/tests/check_tests.cpp (line 391)
<https://reviews.apache.org/r/56213/#comment236898>
s/checkCommandString/command/
src/tests/check_tests.cpp (lines 505 - 515)
<https://reviews.apache.org/r/56213/#comment236900>
this description needs to be updated to reflect the sleep?
src/tests/check_tests.cpp (line 517)
<https://reviews.apache.org/r/56213/#comment236899>
s/checkCommandString/command/
src/tests/check_tests.cpp (lines 673 - 677)
<https://reviews.apache.org/r/56213/#comment236902>
also check for health check status?
- Vinod Kone
On Feb. 8, 2017, 4:56 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56213/
> -----------------------------------------------------------
>
> (Updated Feb. 8, 2017, 4: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
> src/tests/mesos.hpp b450a04dfbf3bbeeb6b605fb78097dca390cbdbe
>
> Diff: https://reviews.apache.org/r/56213/diff/
>
>
> Testing
> -------
>
> See https://reviews.apache.org/r/56218/
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56213: Added check tests for command executor.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56213/
-----------------------------------------------------------
(Updated March 20, 2017, 2:28 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 f035c16920deaf559420ae0d7d881330ff65ae44
src/tests/mesos.hpp f39e243c2c11bc1c9c757049fda2122727d1fef9
Diff: https://reviews.apache.org/r/56213/diff/9/
Changes: https://reviews.apache.org/r/56213/diff/8-9/
Testing
-------
See https://reviews.apache.org/r/56218/
Thanks,
Alexander Rukletsov
Re: Review Request 56213: Added check tests for command executor.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56213/
-----------------------------------------------------------
(Updated March 20, 2017, 11:56 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 (updated)
-----
src/tests/check_tests.cpp f035c16920deaf559420ae0d7d881330ff65ae44
src/tests/mesos.hpp f39e243c2c11bc1c9c757049fda2122727d1fef9
Diff: https://reviews.apache.org/r/56213/diff/8/
Changes: https://reviews.apache.org/r/56213/diff/7-8/
Testing
-------
See https://reviews.apache.org/r/56218/
Thanks,
Alexander Rukletsov
Re: Review Request 56213: Added check tests for command executor.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56213/#review169277
-----------------------------------------------------------
Ship it!
Assuming existing issues are resolved and the file is consistent.
- 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/56213/
> -----------------------------------------------------------
>
> (Updated March 16, 2017, 4:46 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 f035c16920deaf559420ae0d7d881330ff65ae44
> src/tests/mesos.hpp f39e243c2c11bc1c9c757049fda2122727d1fef9
>
>
> Diff: https://reviews.apache.org/r/56213/diff/7/
>
>
> Testing
> -------
>
> See https://reviews.apache.org/r/56218/
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56213: Added check tests for command executor.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56213/
-----------------------------------------------------------
(Updated March 16, 2017, 4:46 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 f035c16920deaf559420ae0d7d881330ff65ae44
src/tests/mesos.hpp f39e243c2c11bc1c9c757049fda2122727d1fef9
Diff: https://reviews.apache.org/r/56213/diff/7/
Changes: https://reviews.apache.org/r/56213/diff/6-7/
Testing
-------
See https://reviews.apache.org/r/56218/
Thanks,
Alexander Rukletsov
Re: Review Request 56213: Added check tests for command executor.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On March 14, 2017, 6:24 p.m., Andrew Schwartzmeyer wrote:
> > src/tests/check_tests.cpp
> > Lines 74 (patched)
> > <https://reviews.apache.org/r/56213/diff/6/?file=1663407#file1663407line74>
> >
> > As the resident PowerShell "expert" (heh!), I tested this (and the second one) and they do what you're expecting.
> >
> > Just a couple notes:
> >
> > You might be wanting to use `Test-Path "somepath"` instead, which returns a boolean for existence of that path, so you can do `if (Test-Path "somepath")` instead of the gunk around checking `$?`. Also, semicolons are unnecssary in PowerShell except to separate multiple commands on one line. If you want it to be _perfect_ we try to prefer the `Camel-Case` of cmdlet names, but it's whatever.
>
> Vinod Kone wrote:
> Thanks Andrew for the reply.
>
> @Alex Do you want to update accordingly?
Sure.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56213/#review168922
-----------------------------------------------------------
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/56213/
> -----------------------------------------------------------
>
> (Updated March 16, 2017, 4:46 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 f035c16920deaf559420ae0d7d881330ff65ae44
> src/tests/mesos.hpp f39e243c2c11bc1c9c757049fda2122727d1fef9
>
>
> Diff: https://reviews.apache.org/r/56213/diff/7/
>
>
> Testing
> -------
>
> See https://reviews.apache.org/r/56218/
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56213: Added check tests for command executor.
Posted by Vinod Kone <vi...@gmail.com>.
> On March 14, 2017, 6:24 p.m., Andrew Schwartzmeyer wrote:
> > src/tests/check_tests.cpp
> > Lines 74 (patched)
> > <https://reviews.apache.org/r/56213/diff/6/?file=1663407#file1663407line74>
> >
> > As the resident PowerShell "expert" (heh!), I tested this (and the second one) and they do what you're expecting.
> >
> > Just a couple notes:
> >
> > You might be wanting to use `Test-Path "somepath"` instead, which returns a boolean for existence of that path, so you can do `if (Test-Path "somepath")` instead of the gunk around checking `$?`. Also, semicolons are unnecssary in PowerShell except to separate multiple commands on one line. If you want it to be _perfect_ we try to prefer the `Camel-Case` of cmdlet names, but it's whatever.
Thanks Andrew for the reply.
@Alex Do you want to update accordingly?
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56213/#review168922
-----------------------------------------------------------
On March 14, 2017, 2:09 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56213/
> -----------------------------------------------------------
>
> (Updated March 14, 2017, 2:09 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 f035c16920deaf559420ae0d7d881330ff65ae44
> src/tests/mesos.hpp f39e243c2c11bc1c9c757049fda2122727d1fef9
>
>
> Diff: https://reviews.apache.org/r/56213/diff/6/
>
>
> Testing
> -------
>
> See https://reviews.apache.org/r/56218/
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56213: Added check tests for command executor.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56213/#review168922
-----------------------------------------------------------
src/tests/check_tests.cpp
Lines 74 (patched)
<https://reviews.apache.org/r/56213/#comment241207>
As the resident PowerShell "expert" (heh!), I tested this (and the second one) and they do what you're expecting.
Just a couple notes:
You might be wanting to use `Test-Path "somepath"` instead, which returns a boolean for existence of that path, so you can do `if (Test-Path "somepath")` instead of the gunk around checking `$?`. Also, semicolons are unnecssary in PowerShell except to separate multiple commands on one line. If you want it to be _perfect_ we try to prefer the `Camel-Case` of cmdlet names, but it's whatever.
- Andrew Schwartzmeyer
On March 14, 2017, 2:09 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56213/
> -----------------------------------------------------------
>
> (Updated March 14, 2017, 2:09 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 f035c16920deaf559420ae0d7d881330ff65ae44
> src/tests/mesos.hpp f39e243c2c11bc1c9c757049fda2122727d1fef9
>
>
> Diff: https://reviews.apache.org/r/56213/diff/6/
>
>
> Testing
> -------
>
> See https://reviews.apache.org/r/56218/
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56213: Added check tests for command executor.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56213/
-----------------------------------------------------------
(Updated March 14, 2017, 2:09 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 f035c16920deaf559420ae0d7d881330ff65ae44
src/tests/mesos.hpp f39e243c2c11bc1c9c757049fda2122727d1fef9
Diff: https://reviews.apache.org/r/56213/diff/6/
Changes: https://reviews.apache.org/r/56213/diff/5-6/
Testing
-------
See https://reviews.apache.org/r/56218/
Thanks,
Alexander Rukletsov
Re: Review Request 56213: Added check tests for command executor.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56213/
-----------------------------------------------------------
(Updated March 7, 2017, 8:39 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 f035c16920deaf559420ae0d7d881330ff65ae44
src/tests/mesos.hpp f39e243c2c11bc1c9c757049fda2122727d1fef9
Diff: https://reviews.apache.org/r/56213/diff/5/
Changes: https://reviews.apache.org/r/56213/diff/4-5/
Testing
-------
See https://reviews.apache.org/r/56218/
Thanks,
Alexander Rukletsov
Re: Review Request 56213: Added check tests for command executor.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56213/#review167860
-----------------------------------------------------------
LGTM modulo addressing the existing issues.
src/tests/check_tests.cpp
Lines 74-79 (patched)
<https://reviews.apache.org/r/56213/#comment239819>
I don't know enough about power shell syntax. Did you confirm with a windows expert that this is the right way to do it? cc @josephwu @mpark @joris?
- Vinod Kone
On March 1, 2017, 12:50 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56213/
> -----------------------------------------------------------
>
> (Updated March 1, 2017, 12:50 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 f035c16920deaf559420ae0d7d881330ff65ae44
> src/tests/mesos.hpp b450a04dfbf3bbeeb6b605fb78097dca390cbdbe
>
>
> Diff: https://reviews.apache.org/r/56213/diff/4/
>
>
> Testing
> -------
>
> See https://reviews.apache.org/r/56218/
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56213: Added check tests for command executor.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56213/
-----------------------------------------------------------
(Updated March 1, 2017, 12:50 p.m.)
Review request for mesos, Gast�n Kleiman and Vinod Kone.
Changes
-------
Addressed comments; added Windows support.
Bugs: MESOS-6906
https://issues.apache.org/jira/browse/MESOS-6906
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
src/tests/check_tests.cpp f035c16920deaf559420ae0d7d881330ff65ae44
src/tests/mesos.hpp b450a04dfbf3bbeeb6b605fb78097dca390cbdbe
Diff: https://reviews.apache.org/r/56213/diff/4/
Changes: https://reviews.apache.org/r/56213/diff/3-4/
Testing
-------
See https://reviews.apache.org/r/56218/
Thanks,
Alexander Rukletsov
Re: Review Request 56213: Added check tests for command executor.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56213/
-----------------------------------------------------------
(Updated Feb. 28, 2017, 3:55 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
src/tests/mesos.hpp b450a04dfbf3bbeeb6b605fb78097dca390cbdbe
Diff: https://reviews.apache.org/r/56213/diff/
Testing
-------
See https://reviews.apache.org/r/56218/
Thanks,
Alexander Rukletsov
Re: Review Request 56213: Added check tests for command executor.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56213/
-----------------------------------------------------------
(Updated Feb. 8, 2017, 4: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
src/tests/mesos.hpp b450a04dfbf3bbeeb6b605fb78097dca390cbdbe
Diff: https://reviews.apache.org/r/56213/diff/
Testing
-------
See https://reviews.apache.org/r/56218/
Thanks,
Alexander Rukletsov
Re: Review Request 56213: Added check tests for command executor.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56213/
-----------------------------------------------------------
(Updated Feb. 8, 2017, 3:18 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
src/tests/mesos.hpp b450a04dfbf3bbeeb6b605fb78097dca390cbdbe
Diff: https://reviews.apache.org/r/56213/diff/
Testing
-------
See https://reviews.apache.org/r/56218/
Thanks,
Alexander Rukletsov