> On Jan. 16, 2013, 10:27 p.m., Ben Mahler wrote: > > Any line limit rules for Java? I've seen 100 and 120 as rules in the past.
this is 80 per sun's conventions which is what hadoop apparently follows, save for the indentation length. http://wiki.apache.org/hadoop/CodeReviewChecklist > On Jan. 16, 2013, 10:27 p.m., Ben Mahler wrote: > > hadoop/mesos/src/java/org/apache/hadoop/mapred/MesosExecutor.java, line 28 > > <https://reviews.apache.org/r/8965/diff/1/?file=248619#file248619line28> > > > > argument formatting that format is per sun's conventions. i'm going to send a formatting only diff later, to make sure our code conforms to hadoop/sun's guidelines. > On Jan. 16, 2013, 10:27 p.m., Ben Mahler wrote: > > hadoop/mesos/src/java/org/apache/hadoop/mapred/MesosExecutor.java, line 33 > > <https://reviews.apache.org/r/8965/diff/1/?file=248619#file248619line33> > > > > fits on one line? nope, the line width is 80 chars. > On Jan. 16, 2013, 10:27 p.m., Ben Mahler wrote: > > hadoop/mesos/src/java/org/apache/hadoop/mapred/MesosExecutor.java, line 59 > > <https://reviews.apache.org/r/8965/diff/1/?file=248619#file248619line59> > > > > LOG calls take a throwable as a second argument, so replace with: > > > > LOG.fatal("Failed to start task tracker", e); > > // Kill the e.printStackTrace(); sweet. > On Jan. 16, 2013, 10:27 p.m., Ben Mahler wrote: > > hadoop/mesos/src/java/org/apache/hadoop/mapred/MesosExecutor.java, line 74 > > <https://reviews.apache.org/r/8965/diff/1/?file=248619#file248619line74> > > > > Strange line wrapping. ? > On Jan. 16, 2013, 10:27 p.m., Ben Mahler wrote: > > hadoop/mesos/src/java/org/apache/hadoop/mapred/MesosScheduler.java, line 145 > > <https://reviews.apache.org/r/8965/diff/1/?file=248620#file248620line145> > > > > This log message doesn't make sense, given the status task ID is > > completed, not pending.. agreed. killed it > On Jan. 16, 2013, 10:27 p.m., Ben Mahler wrote: > > hadoop/mesos/src/java/org/apache/hadoop/mapred/MesosScheduler.java, line 199 > > <https://reviews.apache.org/r/8965/diff/1/?file=248620#file248620line199> > > > > How is this different than not setting the user? this is a required field. > On Jan. 16, 2013, 10:27 p.m., Ben Mahler wrote: > > hadoop/mesos/src/java/org/apache/hadoop/mapred/MesosScheduler.java, line 200 > > <https://reviews.apache.org/r/8965/diff/1/?file=248620#file248620line200> > > > > Do we need the identifier? i'm indifferent, but killed it > On Jan. 16, 2013, 10:27 p.m., Ben Mahler wrote: > > hadoop/mapred-site.xml.patch, line 41 > > <https://reviews.apache.org/r/8965/diff/1/?file=248614#file248614line41> > > > > what is this file:///? Add an explanation? axed this altogether > On Jan. 16, 2013, 10:27 p.m., Ben Mahler wrote: > > hadoop/TUTORIAL.sh, line 629 > > <https://reviews.apache.org/r/8965/diff/1/?file=248608#file248608line629> > > > > We should include a comment with respect to the slaves needing to have > > >= 5 cores, >= X RAM, >= Y DISK. And how they can change that by reducing > > the slots. Even with reducing, we still will require 3 cores. > > > > This might be problematic for people. So a comment here and/or in our > > config patch would be great. this info is now taken from the config file. i've included comments there. - Vinod ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/8965/#review15406 ----------------------------------------------------------- On Jan. 15, 2013, 10:29 p.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/8965/ > ----------------------------------------------------------- > > (Updated Jan. 15, 2013, 10:29 p.m.) > > > Review request for mesos, Benjamin Hindman, Matei Zaharia, 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/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 > >
