> On 2012-02-08 20:11:24, Benjamin Hindman wrote: > >
ben's comments > On 2012-02-08 20:11:24, Benjamin Hindman wrote: > > include/mesos/executor.hpp, line 87 > > <https://reviews.apache.org/r/3761/diff/2/?file=72357#file72357line87> > > > > Okay, I think the right thing here is to pass both SlaveID and > > SlaveInfo. That way we can access the slaveInfo.hostname() and > > slaveInfo.public_hostname() if we need. done..also passing frameworkinfo() instead of executorinfo() to make it easy for slave recovery during restarts. > On 2012-02-08 20:11:24, Benjamin Hindman wrote: > > include/mesos/executor.hpp, line 226 > > <https://reviews.apache.org/r/3761/diff/2/?file=72357#file72357line226> > > > > This looks like I forgot to finish this comment. I think I wanted to > > say: "Note, the executor pointer must outlive the driver." Can you add that > > please? Thanks! done > On 2012-02-08 20:11:24, Benjamin Hindman wrote: > > src/Makefile.am, line 462 > > <https://reviews.apache.org/r/3761/diff/2/?file=72359#file72359line462> > > > > This has been committed in another patch. ok..if that other patch has been pushed to trunk..i will merge. > On 2012-02-08 20:11:24, Benjamin Hindman wrote: > > src/exec/exec.cpp, line 125 > > <https://reviews.apache.org/r/3761/diff/2/?file=72364#file72364line125> > > > > No need for the ':'. done > On 2012-02-08 20:11:24, Benjamin Hindman wrote: > > src/exec/exec.cpp, line 134 > > <https://reviews.apache.org/r/3761/diff/2/?file=72364#file72364line134> > > > > s/for:/for task done > On 2012-02-08 20:11:24, Benjamin Hindman wrote: > > src/exec/exec.cpp, line 135 > > <https://reviews.apache.org/r/3761/diff/2/?file=72364#file72364line135> > > > > Indentation? fixed > On 2012-02-08 20:11:24, Benjamin Hindman wrote: > > src/exec/exec.cpp, line 147 > > <https://reviews.apache.org/r/3761/diff/2/?file=72364#file72364line147> > > > > Wrap line. done > On 2012-02-08 20:11:24, Benjamin Hindman wrote: > > src/exec/exec.cpp, line 162 > > <https://reviews.apache.org/r/3761/diff/2/?file=72364#file72364line162> > > > > This data could be binary! Let's not print it out please. fixed > On 2012-02-08 20:11:24, Benjamin Hindman wrote: > > src/exec/exec.cpp, line 232 > > <https://reviews.apache.org/r/3761/diff/2/?file=72364#file72364line232> > > > > This is more verbose than we need to be, please remove. sorry that was part of debug...removed > On 2012-02-08 20:11:24, Benjamin Hindman wrote: > > src/python/native/proxy_executor.cpp, line 43 > > <https://reviews.apache.org/r/3761/diff/2/?file=72370#file72370line43> > > > > If the rest of the file was updated to spell out the variable (i.e., > > sidObj => slaveIdObj), then do the same here please. done - Vinod ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3761/#review4920 ----------------------------------------------------------- On 2012-02-05 03:34:16, Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/3761/ > ----------------------------------------------------------- > > (Updated 2012-02-05 03:34:16) > > > Review request for mesos, Benjamin Hindman, John Sirois, and Brian Wickman. > > > Summary > ------- > > Currently, the init() callback in Executor is not consistent with the naming > of our callbacks. Like scheduler callback, this should be called > registered(). > > In the near future, we are also going to add a new executor callback called > re-registered() as we implement slaves being restarted without killing > executors. > > As part of this, also got rid of the ExecutorArgs protobuf. > > > Diffs > ----- > > > frameworks/hadoop-0.20.2/src/contrib/mesos/src/java/org/apache/hadoop/mapred/FrameworkExecutor.java > 6744082 > include/mesos/executor.hpp 8e651e7 > include/mesos/mesos.proto b855f70 > src/Makefile.am ac0a209 > src/examples/java/TestExecutor.java 6211490 > src/examples/long_lived_executor.cpp 6ba083b > src/examples/memhog_executor.cpp 333b1d9 > src/examples/test_executor.cpp 9dd244e > src/exec/exec.cpp 48c0d0f > src/java/jni/convert.cpp 04a25dd > src/java/jni/org_apache_mesos_MesosExecutorDriver.cpp b338847 > src/java/src/org/apache/mesos/Executor.java a05591f > src/messages/messages.proto 04dc3a0 > src/python/native/proxy_executor.hpp 3ed9e37 > src/python/native/proxy_executor.cpp 30cf089 > src/python/src/mesos.py 026a611 > src/scaling/nested_exec.py 5c4edf1 > src/sched/sched.cpp 21d670f > src/slave/slave.cpp 9b33a5a > src/tests/fault_tolerance_tests.cpp 7be3d6b > src/tests/master_tests.cpp a411ba5 > src/tests/utils.hpp 660aee9 > > Diff: https://reviews.apache.org/r/3761/diff > > > Testing > ------- > > ./mesos-tests --gtest_filter = -"*Python*" > > All the tests, except the SamplePythonFramework, pass. > > Looking at earlier Jiras, looks like there is a fix for python tests, that > needs to be pushed to the trunk? I'm not sure if that fixes this test fail > yet. > > > Thanks, > > Vinod > >
