> On March 15, 2015, 2:03 a.m., Adam B wrote: > > include/mesos/mesos.proto, lines 321-323 > > <https://reviews.apache.org/r/31539/diff/5/?file=894980#file894980line321> > > > > 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? > > Joerg Schad wrote: > Is it ok if I move this discussion to the follow-up Jira as it is not > really an issue with this patch?
Sure. Definitely not a blocker for this patch, but I would like to see it covered by another JIRA. > On March 15, 2015, 2:03 a.m., Adam B wrote: > > src/tests/master_tests.cpp, line 1928 > > <https://reviews.apache.org/r/31539/diff/5/?file=894989#file894989line1928> > > > > Not yours, but could you help out our style update push and s/> >/>>/ > > in code next to your changes (not necessarily the entire file)? > > Joerg Schad wrote: > As seemingly there is some discussion whether to do such style fixes or > not, I will provide another Patch/Jira to fix > > -> >> for all tests. > > Till Toenshoff wrote: > I would suggest to not do that. Let me try to explain: > > This particual RR is rather unusual in that it touches many files. In > october of last year, we reached a consesus for style debt fixes; we said > that it would be nice if all files touched would also get this style update > (dev-list: `Large changes on the codebase due to MESOS-1872`). > > A. Lets not bundle any style debt fixes with this one at all as it is > atypical and not covered by our consesus. > > B. Lets make two RRs out of it, first fixing all sharp bracket whitspaces > on the files to be touched by this RR. Then base this RR on those style > fixes. This will still allow quick and easy reviews but also get a big pile > of style debt fixes merged into the project. SGTM. > On March 15, 2015, 2:03 a.m., Adam B wrote: > > src/tests/master_tests.cpp, line 2018 > > <https://reviews.apache.org/r/31539/diff/5/?file=894989#file894989line2018> > > > > 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()). > > Joerg Schad wrote: > CreateSlaveFlags also generates a new work_dir assignment, as I restart > the slave I woud like to keep the same work_dir. Sure, but StartSlave(None()) still calls cluster.slaves.start(CreateSlaveFlags()), so it's implicit. ``` Try<PID<slave::Slave> > MesosTest::StartSlave( const Option<slave::Flags>& flags) { return cluster.slaves.start( flags.isNone() ? CreateSlaveFlags() : flags.get()); } ``` - Adam ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31539/#review76502 ----------------------------------------------------------- On March 16, 2015, 3:07 a.m., Joerg Schad wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/31539/ > ----------------------------------------------------------- > > (Updated March 16, 2015, 3:07 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 > >
