mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Vinod Kone <vinodk...@gmail.com>
Subject Re: Review Request 56213: Added check tests for command executor.
Date Fri, 03 Mar 2017 02:29:23 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message