----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/12033/#review22269 -----------------------------------------------------------
Thanks for adding this to the repository!! My memory is a bit blurry for this, so I can't remember all of the context. jenkins/pom.xml <https://reviews.apache.org/r/12033/#comment45739> Should we lower the version number? 0.1? jenkins/src/main/java/org/jenkinsci/plugins/mesos/MesosRetentionStrategy.java <https://reviews.apache.org/r/12033/#comment45742> s/of/of the/ trailing period, and can you add a pointer to the EC2 retention strategy code in question? jenkins/src/main/java/org/jenkinsci/plugins/mesos/MesosRetentionStrategy.java <https://reviews.apache.org/r/12033/#comment45741> whitespace jenkins/src/main/java/org/jenkinsci/plugins/mesos/MesosRetentionStrategy.java <https://reviews.apache.org/r/12033/#comment45743> Can you fix up the indenting in this file? jenkins/src/main/java/org/jenkinsci/plugins/mesos/MesosRetentionStrategy.java <https://reviews.apache.org/r/12033/#comment45744> period jenkins/src/main/java/org/jenkinsci/plugins/mesos/MesosRetentionStrategy.java <https://reviews.apache.org/r/12033/#comment45747> I think the null / empty string case should default to 3 as well if we want 3 as a default. This code is a bit messy though, want to try to clean it up? // We use a default value of 3. this.idleTerminationMinutes = 3; if (idleTerminationMinutes != null) { try { this.idleTerminationMinutes = Integer.parseInt(idleTerminationMinutes); } catch (NumberFormatException e) { Logger.warn("Blah.") } jenkins/src/main/java/org/jenkinsci/plugins/mesos/MesosRetentionStrategy.java <https://reviews.apache.org/r/12033/#comment45749> What does this return? 1 always? jenkins/src/main/java/org/jenkinsci/plugins/mesos/MesosRetentionStrategy.java <https://reviews.apache.org/r/12033/#comment45748> Comment should use // jenkins/src/main/java/org/jenkinsci/plugins/mesos/MesosRetentionStrategy.java <https://reviews.apache.org/r/12033/#comment45750> Can you clean this up? jenkins/src/main/java/org/jenkinsci/plugins/mesos/MesosRetentionStrategy.java <https://reviews.apache.org/r/12033/#comment45751> I don't understand this comment. jenkins/src/main/java/org/jenkinsci/plugins/mesos/MesosRetentionStrategy.java <https://reviews.apache.org/r/12033/#comment45752> Different comment style? jenkins/src/main/java/org/jenkinsci/plugins/mesos/MesosRetentionStrategy.java <https://reviews.apache.org/r/12033/#comment45753> @Override on same line, I'd make the whole function one line, but I don't know how you feel about that stylistically jenkins/src/main/java/org/jenkinsci/plugins/mesos/MesosRetentionStrategy.java <https://reviews.apache.org/r/12033/#comment45754> Can you move this up to the top? and is disabled final? jenkins/src/main/java/org/jenkinsci/plugins/mesos/MesosSlave.java <https://reviews.apache.org/r/12033/#comment45755> finals? jenkins/src/main/java/org/jenkinsci/plugins/mesos/MesosSlave.java <https://reviews.apache.org/r/12033/#comment45756> Why did we need this again, debugging? If so, s/mesos slave/MesosSlave/ jenkins/src/main/java/org/jenkinsci/plugins/mesos/MesosSlave.java <https://reviews.apache.org/r/12033/#comment45758> are these functions overrides? If so, can you add the @Override annotation? jenkins/src/main/java/org/jenkinsci/plugins/mesos/MesosSlave.java <https://reviews.apache.org/r/12033/#comment45757> kill newline jenkins/src/main/java/org/jenkinsci/plugins/mesos/MesosSlave.java <https://reviews.apache.org/r/12033/#comment45759> Instance id == node name? jenkins/src/main/java/org/jenkinsci/plugins/mesos/MesosSlave.java <https://reviews.apache.org/r/12033/#comment45760> kill newline jenkins/src/main/resources/index.jelly <https://reviews.apache.org/r/12033/#comment45740> My memory is blurry, what did we use this for? jenkins/src/main/resources/org/jenkinsci/plugins/mesos/MesosCloud/config.jelly <https://reviews.apache.org/r/12033/#comment45762> whitespace jenkins/src/main/resources/org/jenkinsci/plugins/mesos/MesosSlave/configure-entries.jelly <https://reviews.apache.org/r/12033/#comment45761> whitespace - Ben Mahler On June 21, 2013, 7:18 p.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/12033/ > ----------------------------------------------------------- > > (Updated June 21, 2013, 7:18 p.m.) > > > Review request for mesos, Benjamin Hindman, Ben Mahler, and Jiang Yan Xu. > > > Description > ------- > > This is the initial commit of the Jenkins scheduler. Basically all the code > and proper pom. > > NOTE: Not yet integrated into our build tool chain. > > > This addresses bug MESOS-506. > https://issues.apache.org/jira/browse/MESOS-506 > > > Diffs > ----- > > jenkins/pom.xml PRE-CREATION > jenkins/src/main/java/org/jenkinsci/plugins/mesos/JenkinsScheduler.java > PRE-CREATION > jenkins/src/main/java/org/jenkinsci/plugins/mesos/Mesos.java PRE-CREATION > jenkins/src/main/java/org/jenkinsci/plugins/mesos/MesosCloud.java > PRE-CREATION > jenkins/src/main/java/org/jenkinsci/plugins/mesos/MesosComputer.java > PRE-CREATION > > jenkins/src/main/java/org/jenkinsci/plugins/mesos/MesosComputerLauncher.java > PRE-CREATION > > jenkins/src/main/java/org/jenkinsci/plugins/mesos/MesosRetentionStrategy.java > PRE-CREATION > jenkins/src/main/java/org/jenkinsci/plugins/mesos/MesosSlave.java > PRE-CREATION > jenkins/src/main/java/org/jenkinsci/plugins/mesos/TaskTemplate.java > PRE-CREATION > jenkins/src/main/resources/index.jelly PRE-CREATION > > jenkins/src/main/resources/org/jenkinsci/plugins/mesos/MesosCloud/computerSet.jelly > PRE-CREATION > > jenkins/src/main/resources/org/jenkinsci/plugins/mesos/MesosCloud/config.jelly > PRE-CREATION > > jenkins/src/main/resources/org/jenkinsci/plugins/mesos/MesosCloud/help-master.html > PRE-CREATION > > jenkins/src/main/resources/org/jenkinsci/plugins/mesos/MesosSlave/configure-entries.jelly > PRE-CREATION > > jenkins/src/main/resources/org/jenkinsci/plugins/mesos/TaskTemplate/config.jelly > PRE-CREATION > > Diff: https://reviews.apache.org/r/12033/diff/ > > > Testing > ------- > > MESOS_NATIVE_LIBRARY=/Users/vinod/workspace/apache/mesos/build/src/.libs/libmesos.dylib > JAVA_HOME=/Library/Java/JavaVirtualMachines/jdk1.7.0_07.jdk/Contents/Home > mvn hpi:run > > > Thanks, > > Vinod Kone > >
