----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17686/#review33969 -----------------------------------------------------------
Jie and I went through a large chunk of this, here's what we have not yet looked at: src/log: jie will take a look src/master: I'll take a look src/state: jie and I will look src/zookeeper: yan took a look monitor.cpp: ResourceMonitorProcess::_collect looks at a discarded result, but this is not actually possible (no one will discard this). We should consider logging an error when this occurs. ResourceMonitorProcess::__statistics skips discarded, but discarded should not occur, should this return a Failure? sched.cpp: In authenticationTimeout(), we should update the log message to reflect that we're only _attempting_ to discard the authentication, since it could still succeed, despite our discard() call. cgroups.cpp: We commented on how to clean up the EventListener. There were bugs in the Freezer, and EmptyWatcher. src/linux/cgroups.cpp <https://reviews.apache.org/r/17686/#comment63852> Move this to the bottom and: // Discard reading if the caller discards. promise.future().onDiscard(lambda::bind(__discard, reading)); src/linux/cgroups.cpp <https://reviews.apache.org/r/17686/#comment63851> virtual void finalize() { // Unregister the eventfd if needed. if (eventfd.isSome()) { Try<Nothing> unregister = internal::unregisterNotifier(eventfd.get()); if (unregister.isError()) { LOG(ERROR) << "Failed to unregistering eventfd: " << unregister.error(); } } } src/linux/cgroups.cpp <https://reviews.apache.org/r/17686/#comment63854> Remove this. src/linux/cgroups.cpp <https://reviews.apache.org/r/17686/#comment63853> Now we discard the promise only if reading is discarded: if (reading.isDiscarded()) { promise.discard(); } else if (reading.isFailed()) { ... src/linux/cgroups.cpp <https://reviews.apache.org/r/17686/#comment63856> This is a bug, we need to discard the promise! src/linux/cgroups.cpp <https://reviews.apache.org/r/17686/#comment63857> This is a bug, we need to discard the future! (Doing so in finalize() should be sufficient). src/linux/cgroups.cpp <https://reviews.apache.org/r/17686/#comment63860> // Discard the chain when the caller discards. promise.future().onDiscard(lambda::bind(__discard, chain)); src/linux/cgroups.cpp <https://reviews.apache.org/r/17686/#comment63861> We no longer need finalize() with our other comments applied. src/linux/cgroups.cpp <https://reviews.apache.org/r/17686/#comment63858> Remove the CHECK for discarded. src/linux/cgroups.cpp <https://reviews.apache.org/r/17686/#comment63859> if (empty.isDiscarded()) { promise.discard(); terminate(self()); } ... src/linux/cgroups.cpp <https://reviews.apache.org/r/17686/#comment63874> We thought through a fix here but it's non trivial, you can use await() instead of collect() but really it seems since a partial destroy is likely, this should be expressed in the Future.. src/linux/cgroups.cpp <https://reviews.apache.org/r/17686/#comment63869> if (promise.future().hasDiscard()) { // TODO: Remove cgroups for those that were not discarded. foreach (const Future<bool>& kill_, kill.get()) { if (kill_.isDiscarded()) { promise.discard(); terminate(self()); return; } } } if (kill.isReady()) { ... } else if (kill.isFailed()) { // TODO: Remove cgroups for those kills that did not fail. ... } src/sasl/authenticatee.hpp <https://reviews.apache.org/r/17686/#comment63846> Would be nice to document the discard semantics in the API. :) src/sasl/authenticatee.hpp <https://reviews.apache.org/r/17686/#comment63844> This seems a little strange: STATUS=ERROR -> fail() STATUS=DISCARDED -> fail() (why not discard())? Also this looks a little odd to the callers as well, since they discard but a failure will be the result. src/sasl/authenticator.hpp <https://reviews.apache.org/r/17686/#comment63848> No documentation?? ;) Let's at least document the authenticate() call, including the discard semantics. src/sasl/authenticator.hpp <https://reviews.apache.org/r/17686/#comment63847> Ditto for DISCARDED -> fail() here. src/slave/gc.cpp <https://reviews.apache.org/r/17686/#comment63821> We're not handling a 'discard' of the Future returned by schedule(), we should either: -Unschedule the operation when the caller discards the Future, or -Document in gc.hpp that a discard is a no-op (since the slave does not discard, this would be ok for now). src/slave/gc.cpp <https://reviews.apache.org/r/17686/#comment63822> We should document in gc.hpp that an unschedule operation cannot be discarded, no? - Ben Mahler On Feb. 5, 2014, 11:41 p.m., Benjamin Hindman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/17686/ > ----------------------------------------------------------- > > (Updated Feb. 5, 2014, 11:41 p.m.) > > > Review request for mesos, Adam B, Ben Mahler, Ian Downes, Jie Yu, Niklas > Nielsen, TILL TOENSHOFF, Vinod Kone, and Jiang Yan Xu. > > > Repository: mesos-git > > > Description > ------- > > See summary. > > > Diffs > ----- > > src/java/jni/org_apache_mesos_state_AbstractState.cpp > 2ee0b1b631b80ec783e6bce683cdeaa77e56b2aa > src/linux/cgroups.cpp 19ab1f348191ab0315271477b206aa8c6456fd5a > src/log/catchup.cpp 4ee32f285f77eb2de661e22a301b743bb8a06f9c > src/log/consensus.cpp b89673a3b8f233e901eaf9ae69a9979099f4eb73 > src/log/recover.cpp bb32e51e1172a8b32ac74be9848c1e72db5cefa0 > src/master/detector.cpp 2b169c551affe7acd2feac7806a27b46eb99bb88 > src/master/registrar.cpp 915885a160f790399e8185c28c6e6555af1ee76e > src/sasl/authenticatee.hpp f1a677f8aed0979f958e51f85e0a8210a03bd343 > src/sasl/authenticator.hpp 1478f6771b424555c34586a0d61f208dc15b0e7d > src/slave/gc.cpp 405350bf8f498d2e59e9e6b4c4c19b7bdaa974de > src/zookeeper/contender.cpp 6710da4e64fc0a43c1eabfc0f39fb0133c13df14 > src/zookeeper/group.cpp ecb6c002e8194b8d67e262826d988f747414f9f3 > > Diff: https://reviews.apache.org/r/17686/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Benjamin Hindman > >
