----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/8965/#review15674 -----------------------------------------------------------
hadoop/mesos/src/java/org/apache/hadoop/mapred/MesosExecutor.java <https://reviews.apache.org/r/8965/#comment33805> This reads strangely: Setting config option : mesos.master : local How about: Setting config option 'mesos.master' to 'local' hadoop/mesos/src/java/org/apache/hadoop/mapred/MesosExecutor.java <https://reviews.apache.org/r/8965/#comment33814> fatal, and did you want to use the "Failed to start..." format? hadoop/mesos/src/java/org/apache/hadoop/mapred/MesosExecutor.java <https://reviews.apache.org/r/8965/#comment33806> kill this unnecessary e.printStackTrace()? hadoop/mesos/src/java/org/apache/hadoop/mapred/MesosExecutor.java <https://reviews.apache.org/r/8965/#comment33813> fatal hadoop/mesos/src/java/org/apache/hadoop/mapred/MesosExecutor.java <https://reviews.apache.org/r/8965/#comment33807> ditto hadoop/mesos/src/java/org/apache/hadoop/mapred/MesosScheduler.java <https://reviews.apache.org/r/8965/#comment33808> You can kill my TODO here, now that you've added the slot options. hadoop/mesos/src/java/org/apache/hadoop/mapred/MesosScheduler.java <https://reviews.apache.org/r/8965/#comment33809> Can you update this comment with the correct numbers? Also, you can change this from a TODO to a NOTE. hadoop/mesos/src/java/org/apache/hadoop/mapred/MesosScheduler.java <https://reviews.apache.org/r/8965/#comment33810> These can now become final again, since we no longer change them for local runs. hadoop/mesos/src/java/org/apache/hadoop/mapred/MesosScheduler.java <https://reviews.apache.org/r/8965/#comment33811> kill the '.' hadoop/mesos/src/java/org/apache/hadoop/mapred/MesosScheduler.java <https://reviews.apache.org/r/8965/#comment33812> Given the amount of nested scope here (8 close braces), can you re-structure this a bit? jobUpdated(...) { JobInProgress job = event.getJobInProgress(); if (!job.isComplete()) return; LOG.info("Completed job : " + job.getJobID()); synchronized (MesosScheduler.this) { // the kill logic } } hadoop/mesos/src/java/org/apache/hadoop/mapred/MesosScheduler.java <https://reviews.apache.org/r/8965/#comment33815> Could also be expired. hadoop/mesos/src/java/org/apache/hadoop/mapred/MesosScheduler.java <https://reviews.apache.org/r/8965/#comment33816> so null is handled? seems like it would expect an empty list returned intead.. hadoop/mesos/src/java/org/apache/hadoop/mapred/MesosScheduler.java <https://reviews.apache.org/r/8965/#comment33817> s/Compute/Mark hadoop/mesos/src/java/org/apache/hadoop/mapred/MesosScheduler.java <https://reviews.apache.org/r/8965/#comment33818> launch excessive trackers hadoop/mesos/src/java/org/apache/hadoop/mapred/MesosScheduler.java <https://reviews.apache.org/r/8965/#comment33819> I think the logic here is incorrect on this latest review. We want to know about the "available" slots. That is, slots that are not currently in use. (status.getAvailableMapSlots()). Whereas here we consider all launched slots as available, regardless of whether or not they are in use. So.. to fix this: kill the launched vars here and the loop above becomes: idleMapSlots = 0; idleReduceSlots = 0; for (TaskTrackerStatus status : jobTracker.taskTrackers()) { HttpHost host = new HttpHost(status.getHost(), status.getHttpPort()); if (mesosTrackers.containsKey(host)) { mesosTrackers.get(host).active = true; idleMapSlots += status.getAvailableMapSlots(); idleReduceSlots += status.getAvailableReduceSlots(); } } make sense? hadoop/mesos/src/java/org/apache/hadoop/mapred/MesosScheduler.java <https://reviews.apache.org/r/8965/#comment33820> I think a switch (with all the states) is clearer here, but up to you. - Ben Mahler On Jan. 25, 2013, 2:32 a.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/8965/ > ----------------------------------------------------------- > > (Updated Jan. 25, 2013, 2:32 a.m.) > > > Review request for mesos, Benjamin Hindman and Ben Mahler. > > > Description > ------- > > Features: > > --> Mesos scheduler is a thin shim that delegates the actual scheduling of > task trackers to the underlying scheduler. > --> Mesos executor is a thin shim that starts/stops task tracker properly. > --> Doesn't depend on mesos specific patches (status updates, task tracker > instrumentation) for hadoop. > --> Builds mesos executor bundle that can be used when running on a real > mesos cluster. > > > Diffs > ----- > > hadoop/Makefile.am 46d16f7c03cda217d1851d78515cad96f60cba2b > hadoop/NOTES PRE-CREATION > hadoop/TUTORIAL.sh fca22c5b7bd485a5d18c99cb6666fab6bb0a06bb > hadoop/hadoop-0.20.2-cdh3u3.patch 84a948f23122a25fb41dfe497a73bff0c8e0867d > hadoop/hadoop-0.20.2-cdh3u3_hadoop-env.sh.patch > b1731e27b1f59d8dc913d259d8f32a69f46ac0b0 > hadoop/hadoop-0.20.2-cdh3u3_mesos.patch > f93e53e286525829bdc2a3da71eefab8f093e5b8 > hadoop/hadoop-0.20.205.0.patch cab488e402343bdbd5d0c4101785b9a48331f85e > hadoop/hadoop-0.20.205.0_hadoop-env.sh.patch > 1b8f4d06323a3ca3f624d9374066e29be5a18acd > hadoop/hadoop-7698-1.patch PRE-CREATION > hadoop/mapred-site.xml.patch c2d6397d8fc1d662afd606e1069ff469c3556e19 > hadoop/mesos-executor ce8dcccf7eaecd35dfa041a78aa9760dc2ce451c > hadoop/mesos/src/java/org/apache/hadoop/mapred/FrameworkExecutor.java > 9238fe53a718c47ff4bbe2d555d1c072fa8bd6e2 > hadoop/mesos/src/java/org/apache/hadoop/mapred/FrameworkScheduler.java > 754b198a5c7baacc92ce2adc96da15aa97c47ad1 > hadoop/mesos/src/java/org/apache/hadoop/mapred/HadoopFrameworkMessage.java > 1e9510873bab962250afbaf81a5c63a6b191e299 > hadoop/mesos/src/java/org/apache/hadoop/mapred/MesosExecutor.java > PRE-CREATION > hadoop/mesos/src/java/org/apache/hadoop/mapred/MesosScheduler.java > 1408f300a43eab043b40e4d313e6632cb21fd7b2 > > hadoop/mesos/src/java/org/apache/hadoop/mapred/MesosTaskTrackerInstrumentation.java > 8c517d80e54555e12f906a219b12eb572ed7e3a3 > > Diff: https://reviews.apache.org/r/8965/diff/ > > > Testing > ------- > > make hadoop-0.20.2-cdh3u3 > make hadoop-0.20.205.0 > > Also, verified that the generator executor bundle is self-contained and works > as expected for non-local runs. > > > Thanks, > > Vinod Kone > >
