mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Adam B" <a...@mesosphere.io>
Subject Re: Review Request 31539: Remove the checkpoint variable entirely from slave/flags.hpp.
Date Sun, 15 Mar 2015 09:03:55 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31539/#review76502
-----------------------------------------------------------


First pass looks good. I've got a couple of questions about the follow-up removal of SlaveInfo.checkpoint,
and then some style nits and general cleanup requests. I still need to go through the actual
test logic itself to understand those changes, but the core change looks good.


include/mesos/mesos.proto
<https://reviews.apache.org/r/31539/#comment124041>

    Style nit: s/remove/Remove/



include/mesos/mesos.proto
<https://reviews.apache.org/r/31539/#comment124043>

    Will we be able to remove this flag in 0.23, or will we need to wait for another release
cycle for deprecation? Seems like if it was already 'optional' in 0.22, and it was never set/saved
as true in 0.22, then we could remove it in 0.23, as an 0.23 slave recovering 0.22 SlaveInfo
would be fine. What about an 0.22 slave registering with an 0.23 master, and vice versa? How
safe would that be?
    Maybe we need to change the default to true here?



include/mesos/mesos.proto
<https://reviews.apache.org/r/31539/#comment124042>

    s/Mesos/MESOS/



src/slave/slave.cpp
<https://reviews.apache.org/r/31539/#comment124045>

    s/as to be/to be/



src/slave/slave.cpp
<https://reviews.apache.org/r/31539/#comment124044>

    Minor: Technically, "Delete after 0.23.", since you cannot complete the deprecation during
0.23.1 or another point release.



src/tests/docker_containerizer_tests.cpp
<https://reviews.apache.org/r/31539/#comment124046>

    All of these lines can be removed like you did above, right?



src/tests/gc_tests.cpp
<https://reviews.apache.org/r/31539/#comment124047>

    s/  / / (remove double-space), and you can also s/the garbage/garbage/



src/tests/master_authorization_tests.cpp
<https://reviews.apache.org/r/31539/#comment124048>

    s/  / / (remove double-space)



src/tests/master_tests.cpp
<https://reviews.apache.org/r/31539/#comment124049>

    s/FLags/flags/



src/tests/master_tests.cpp
<https://reviews.apache.org/r/31539/#comment124050>

    s/> >/>>/



src/tests/master_tests.cpp
<https://reviews.apache.org/r/31539/#comment124051>

    Remove the now-unecessary comment, and these default-value initializations (as you did
elsewhere).



src/tests/master_tests.cpp
<https://reviews.apache.org/r/31539/#comment124052>

    Not yours, but could you help out our style update push and s/> >/>>/ in code
next to your changes (not necessarily the entire file)?



src/tests/master_tests.cpp
<https://reviews.apache.org/r/31539/#comment124053>

    Do we even need to CreateSlaveFlags() here and elsewhere? If you're not setting any non-default
flag values or otherwise using the 'slaveFlags' variable, it can be removed, since StartSlave()
is the same as StartSlave(CreateSlaveFlags()).



src/tests/partition_tests.cpp
<https://reviews.apache.org/r/31539/#comment124054>

    Remove the CreateSlaveFlags() line if nothing else uses 'flags'.
    s/> >/>>/



src/tests/slave_recovery_tests.cpp
<https://reviews.apache.org/r/31539/#comment124055>

    Should be able to remove all of these too, right? Then you can just directly `return ContainerizerTest<T>::CreateSlaveFlags();`


- Adam B


On March 14, 2015, 7:10 a.m., Joerg Schad wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31539/
> -----------------------------------------------------------
> 
> (Updated March 14, 2015, 7:10 a.m.)
> 
> 
> Review request for mesos, Adam B, Cody Maloney, and Till Toenshoff.
> 
> 
> Bugs: MESOS-2375
>     https://issues.apache.org/jira/browse/MESOS-2375
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> As a number of tests rely on the checkpointing flag to be false, a few tests had to be
adapted.
> Removed the following test as the tested logic is specific to (old) non-checkpointing
slaves:
> SlaveRecoveryTest.NonCheckpointingSlave:
>    This test checks whether a non-checkpointing slave is not scheduled to a checkpointing
framework.   
>    It can be removed as all slaves are checkpointing slaves.
> 
> 
> Diffs
> -----
> 
>   include/mesos/mesos.proto 9df972d750ce1e4a81d2e96cc508d6f83cad2fc8 
>   src/slave/flags.hpp 56b25caf3901b38bdecb50310e8bcae0b114efa8 
>   src/slave/slave.cpp 0f99e4efb8fa2b96f120a3e49191158ca0364c06 
>   src/tests/disk_quota_tests.cpp 9c3a8815c3478535b72888c296a4aa5cda341ba3 
>   src/tests/docker_containerizer_tests.cpp 06cd3d89ecbaaac17ae6970604b21fbe29f6e887 
>   src/tests/fault_tolerance_tests.cpp 9ac75b1f601e14a3d3d117775f37a4a48b291dc6 
>   src/tests/gc_tests.cpp deaa6b1b6c32ae6d153229248d7d4f57caa0ebcf 
>   src/tests/master_allocator_tests.cpp a432d0207e1a92532a495bf9ad2826414ee4f6f0 
>   src/tests/master_authorization_tests.cpp ff706ed6f8537207b30a548b0ce2121c5df71ab9 
>   src/tests/master_tests.cpp e69348be676a80017062e3abbd15b8008a6009d7 
>   src/tests/master_validation_tests.cpp c8742928a4e93e86ccd0f5a39856a65cfe8eb74f 
>   src/tests/mesos.cpp c8f43d21b214e75eaac2870cbdf4f03fd18707d1 
>   src/tests/partition_tests.cpp bb96aed37861867fbde68445016f0c6e039f3fb4 
>   src/tests/persistent_volume_tests.cpp b617117ade4b487cc06002cfeca76a0486833b20 
>   src/tests/reconciliation_tests.cpp acd70021574b05ab23872add5bdfa4a46b7dfc51 
>   src/tests/slave_recovery_tests.cpp 53adae0118a26e6d25a9ff20c6374cc8e73275b1 
>   src/tests/status_update_manager_tests.cpp 216a22e9f292b4141c8b966dad0f25dbd791c025

> 
> Diff: https://reviews.apache.org/r/31539/diff/
> 
> 
> Testing
> -------
> 
> make check GTEST_BREAK_ON_FAILURE=1 GTEST_SHUFFLE=1 GTEST_REPEAT=50 on OSX (had to exclude
some known flaky tests under OSX)
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>


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