Merge branch 'pull-743'

core/src/main/grammar/crontab.g

@@ -93,7 +93,7 @@ term [int field]
 returns [long bits=0]
 throws ANTLRException
 {
-  int d=1,s,e,t;
+  int d=NO_STEP,s,e,t;
 }
   : (token "-")=> s=token "-" e=token ( "/" d=token )?
   {
@@ -108,13 +108,13 @@ throws ANTLRException
   {
     bits = doRange(d,field);
   }
-  | ("H" "(")=> "H" "(" s=token "-" e=token ")"
+  | ("H" "(")=> "H" "(" s=token "-" e=token ")" ( "/" d=token )?
   {
-    bits = doHash(s,e);
+    bits = doHash(s,e,d);
   }
-  | "H"
+  | "H" ( "/" d=token )?
   {
-    bits = doHash(field);
+    bits = doHash(d,field);
   }
   ;
 

core/src/main/java/hudson/scheduler/BaseParser.java

@@ -87,17 +87,37 @@ abstract class BaseParser extends LLkParser {
 
     /**
      * Uses {@link Hash} to choose a random (but stable) value from within this field.
+     *
+     * @param step
+     *      Increments. For example, 15 if "H/15". Or {@link #NO_STEP} to indicate
+     *      the special constant for "H" without the step value.
      */
-    protected long doHash( int field ) {
+    protected long doHash(int step, int field) throws ANTLRException {
         int u = UPPER_BOUNDS[field];
         if (field==2) u = 28;   // day of month can vary depending on month, so to make life simpler, just use [1,28] that's always safe
         if (field==4) u = 6;   // Both 0 and 7 of day of week are Sunday. For better distribution, limit upper bound to 6
-        int h = hash.next(u+1 - LOWER_BOUNDS[field]); // upper bound is inclusive
-        return 1L << (h+LOWER_BOUNDS[field]);
+        return doHash(LOWER_BOUNDS[field], u, step);
     }
 
-    protected long doHash( int s, int e ) {
-        return 1L << (s+hash.next(e+1-s));
+    protected long doHash(int s, int e, int step) throws ANTLRException {
+        if (step > e - s + 1) {
+            error(Messages.BaseParser_OutOfRange(step, 1, e - s + 1));
+            throw new AssertionError();
+        } else if (step > 1) {
+            long bits = 0;
+            for (int i = hash.next(step) + s; i <= e; i += step) {
+                bits |= 1L << i;
+            }
+            assert bits != 0;
+            return bits;
+        } else if (step <=0) {
+            error(Messages.BaseParser_MustBePositive(step));
+            throw new AssertionError();
+        } else {
+            assert step==NO_STEP;
+            // step=1 (i.e. omitted) in the case of hash is actually special; means pick one value, not step by 1
+            return 1L << (s+hash.next(e+1-s));
+        }
     }
     
     protected void rangeCheck(int value, int field) throws ANTLRException {
@@ -122,7 +142,11 @@ abstract class BaseParser extends LLkParser {
 
     /**
      * This property hashes tokens in the cron tab tokens like @daily so that they spread evenly.
-     * This is more aggressive optimization that changes the semantics, so not on by default.
      */
-    public static boolean HASH_TOKENS = Boolean.getBoolean(BaseParser.class.getName()+".hash");
+    public static boolean HASH_TOKENS = !"false".equals(System.getProperty(BaseParser.class.getName()+".hash"));
+
+    /**
+     * Constant that indicates no step value.
+     */
+    public static final int NO_STEP = 1;
 }

core/src/main/java/hudson/scheduler/CronTab.java

@@ -31,6 +31,7 @@ import java.util.GregorianCalendar;
 import java.util.Locale;
 
 import static java.util.Calendar.*;
+import javax.annotation.CheckForNull;
 
 /**
  * Table for driving scheduled tasks.
@@ -412,21 +413,45 @@ public final class CronTab {
      * but semantically suspicious combinations, like
      * "* 0 * * *"
      */
-    public String checkSanity() {
-        for( int i=0; i<5; i++ ) {
+    public @CheckForNull String checkSanity() {
+        OUTER: for (int i = 0; i < 5; i++) {
             long bitMask = (i<4)?bits[i]:(long)dayOfWeek;
             for( int j=BaseParser.LOWER_BOUNDS[i]; j<=BaseParser.UPPER_BOUNDS[i]; j++ ) {
                 if(!checkBits(bitMask,j)) {
                     // this rank has a sparse entry.
                     // if we have a sparse rank, one of them better be the left-most.
                     if(i>0)
-                        return Messages.CronTab_do_you_really_mean_every_minute_when_you(spec, "0 " + spec.substring(spec.indexOf(' ')+1));
+                        return Messages.CronTab_do_you_really_mean_every_minute_when_you(spec, "H " + spec.substring(spec.indexOf(' ') + 1));
                     // once we find a sparse rank, upper ranks don't matter
-                    return null;
+                    break OUTER;
                 }
             }
         }
 
+        String hashified = hashify(spec);
+        if (hashified != null) {
+            return Messages.CronTab_spread_load_evenly_by_using_rather_than_(hashified, spec);
+        }
+
         return null;
     }
+
+    /**
+     * Checks a prospective crontab specification to see if it could benefit from balanced hashes.
+     * @param spec a (legal) spec
+     * @return a similar spec that uses a hash, if such a transformation is necessary; null if it is OK as is
+     * @since 1.509
+     */
+    public static @CheckForNull String hashify(String spec) {
+        if (spec.contains("H")) {
+            // if someone is already using H, presumably he knows what it is, so a warning is likely false positive
+            return null;
+        } else if (spec.startsWith("*/")) {// "*/15 ...." (every N minutes) to hash
+            return "H" + spec.substring(1);
+        } else if (spec.matches("\\d+ .+")) {// "0 ..." (certain minute) to hash
+            return "H " + spec.substring(spec.indexOf(' ') + 1);
+        } else {
+            return null;
+        }
+    }
 }

core/src/main/resources/hudson/scheduler/Messages.properties

@@ -24,4 +24,5 @@ BaseParser.StartEndReversed=You mean {0}-{1}?
 BaseParser.MustBePositive=step must be positive, but found {0}
 BaseParser.OutOfRange={0} is an invalid value. Must be within {1} and {2}
 CronTab.do_you_really_mean_every_minute_when_you=Do you really mean "every minute" when you say "{0}"? Perhaps you meant "{1}"
+CronTab.spread_load_evenly_by_using_rather_than_=Spread load evenly by using \u2018{0}\u2019 rather than \u2018{1}\u2019
 CronTabList.InvalidInput=Invalid input: "{0}": {1}

core/src/main/resources/hudson/triggers/TimerTrigger/help-spec.html

@@ -5,23 +5,23 @@
   <table>
     <tr>
       <td>MINUTE</td>
-      <td>Minutes within the hour (0-59)</td>
+      <td>Minutes within the hour (0–59)</td>
     </tr>
     <tr>
       <td>HOUR</td>
-      <td>The hour of the day (0-23)</td>
+      <td>The hour of the day (0–23)</td>
     </tr>
     <tr>
       <td>DOM</td>
-      <td>The day of the month (1-31)</td>
+      <td>The day of the month (1–31)</td>
     </tr>
     <tr>
       <td>MONTH</td>
-      <td>The month (1-12)</td>
+      <td>The month (1–12)</td>
     </tr>
     <tr>
       <td>DOW</td>
-      <td>The day of the week (0-7) where 0 and 7 are Sunday.</td>
+      <td>The day of the week (0–7) where 0 and 7 are Sunday.</td>
     </tr>
   </table>
   <p>
@@ -29,44 +29,47 @@
     available. In the order of precedence,
   </p>
   <ul>
-    <li>'*' can be used to specify all valid values.</li>
-    <li>'M-N' can be used to specify a range, such as "1-5"</li>
-    <li>'M-N/X' or '*/X' can be used to specify skips of X's value through the range,
-        such as "*/15" in the MINUTE field for "0,15,30,45" and "1-6/2" for "1,3,5"</li>
-    <li>'A,B,...,Z' can be used to specify multiple values, such as "0,30" or "1,3,5"</li>
+    <li><code>*</code> specifies all valid values</li>
+    <li><code>M-N</code> specifies a range of values</li>
+    <li><code>M-N/X</code> or <code>*/X</code> steps by intervals of X through the specified range or whole valid range</li>
+    <li><code>A,B,...,Z</code> enumerates multiple values</li>
   </ul>
   <p>
     To allow periodically scheduled tasks to produce even load on the system,
-    the '<tt>H</tt>' token can be used. For example, people often use
-    '<tt>0 0 * * *</tt>' for a daily job, but this ends up causing a large
-    spike in midnight. In contrast, doing '<tt>H H * * *</tt>' would
-    still execute a job once a day, but the actual time of the day this gets
-    executed will be spread over by Jenkins.
+    the symbol <code>H</code> (for “hash”) should be used wherever possible.
+    For example, using <code>0 0 * * *</code> for a dozen daily jobs
+    will cause a large spike at midnight.
+    In contrast, using <code>H H * * *</code> would still execute each job once a day,
+    but not all at the same time, better using limited resources.
   </p><p>
-    The 'H' token can be used with a range. For example, '<tt>H H(0-7) * * *</tt>'
-    means some time between midnight to 7:59am.
+    The <code>H</code> symbol can be used with a range. For example, <code>H H(0-7) * * *</code>
+    means some time between 12:00 AM (midnight) to 7:59 AM.
+    You can also use step intervals with <code>H</code>, with or without ranges.
   </p><p>
-    The '<tt>H</tt>' token can be thought of as a random value over a range,
+    The <code>H</code> symbol can be thought of as a random value over a range,
     but it actually is a hash of the job name, not a random function, so that
     the value remains stable for any given project.
   </p>
   <p>
-    Empty lines and lines that start with '#' will be ignored as comments.
+    Empty lines and lines that start with <code>#</code> will be ignored as comments.
   </p><p>
-    In addition, '@yearly', '@annually', '@monthly', '@weekly', '@daily', '@midnight',
-    and '@hourly' are supported.
+    In addition, <code>@yearly</code>, <code>@annually</code>, <code>@monthly</code>,
+    <code>@weekly</code>, <code>@daily</code>, <code>@midnight</code>,
+    and <code>@hourly</code> are supported as convenient aliases.
+    These use the hash system for automatic balancing.
+    For example, <code>@hourly</code> is the same as <code>H * * * *</code> and could mean at any time during the hour.
+    <code>@midnight</code> actually means some time between 12:00 AM and 2:59 AM.
+  </p><p>
+    Examples:
   </p>
-  <table>
-    <tr>
-      <td>Examples</td>
-      <td>
 <pre>
-# every minute
-* * * * *
-# every 5 mins past the hour 
-5 * * * *
+# every fifteen minutes (perhaps at :07, :22, :37, :52)
+H/15 * * * *
+# every ten minutes in the first half of every hour (three times, perhaps at :04, :14, :24)
+H(0-29)/10 * * * *
+# once every two hours every weekday (perhaps at 10:38 AM, 12:38 PM, 2:38 PM, 4:38 PM)
+H 9-16/2 * * 1-5
+# once a day on the 1st and 15th of every month except December
+H H 1,15 1-11 *
 </pre>
-      </td>
-    </tr>
-  </table>
-</div>
\ No newline at end of file
+</div>

core/src/test/java/hudson/scheduler/CronTabTest.java

@@ -35,17 +35,12 @@ import org.jvnet.hudson.test.Bug;
 import org.jvnet.hudson.test.Url;
 
 import static java.util.Calendar.MONDAY;
-import org.junit.BeforeClass;
 
 /**
  * @author Kohsuke Kawaguchi
  */
 public class CronTabTest {
 
-    @BeforeClass public static void hashTokens() {
-        BaseParser.HASH_TOKENS = true;
-    }
-
     @Test
     public void test1() throws ANTLRException {
         new CronTab("@yearly");
@@ -179,14 +174,21 @@ public class CronTabTest {
     }
 
     @Test public void checkSanity() throws Exception {
-        assertEquals(Messages.CronTab_do_you_really_mean_every_minute_when_you("* * * * *", "0 * * * *"), new CronTab("* * * * *").checkSanity());
-        assertEquals(null, new CronTab("0 * * * *").checkSanity());
-        assertEquals(null, new CronTab("0 3 * * *").checkSanity());
+        assertEquals(null, new CronTab("@hourly").checkSanity());
+        assertEquals(Messages.CronTab_do_you_really_mean_every_minute_when_you("* * * * *", "H * * * *"), new CronTab("* * * * *").checkSanity());
         assertEquals(null, new CronTab("H H(0-2) * * *", Hash.from("stuff")).checkSanity());
-        assertEquals(Messages.CronTab_do_you_really_mean_every_minute_when_you("* 0 * * *", "0 0 * * *"), new CronTab("* 0 * * *").checkSanity());
-        assertEquals(Messages.CronTab_do_you_really_mean_every_minute_when_you("* 6,18 * * *", "0 6,18 * * *"), new CronTab("* 6,18 * * *").checkSanity());
+        assertEquals(Messages.CronTab_do_you_really_mean_every_minute_when_you("* 0 * * *", "H 0 * * *"), new CronTab("* 0 * * *").checkSanity());
+        assertEquals(Messages.CronTab_do_you_really_mean_every_minute_when_you("* 6,18 * * *", "H 6,18 * * *"), new CronTab("* 6,18 * * *").checkSanity());
         // dubious; could be improved:
-        assertEquals(Messages.CronTab_do_you_really_mean_every_minute_when_you("* * 3 * *", "0 * 3 * *"), new CronTab("* * 3 * *").checkSanity());
+        assertEquals(Messages.CronTab_do_you_really_mean_every_minute_when_you("* * 3 * *", "H * 3 * *"), new CronTab("* * 3 * *").checkSanity());
+        // promote hashes:
+        assertEquals(Messages.CronTab_spread_load_evenly_by_using_rather_than_("H/15 * * * *", "*/15 * * * *"), new CronTab("*/15 * * * *").checkSanity());
+        // XXX 0,15,30,45 * * * * → H/15 * * * *
+        assertEquals(Messages.CronTab_spread_load_evenly_by_using_rather_than_("H * * * *", "0 * * * *"), new CronTab("0 * * * *").checkSanity());
+        // if the user specifically asked for 3:00 AM, probably we should stick to 3:00–3:59
+        assertEquals(Messages.CronTab_spread_load_evenly_by_using_rather_than_("H 3 * * *", "0 3 * * *"), new CronTab("0 3 * * *").checkSanity());
+
+        assertEquals(null, new CronTab("H/15 * 1 1 *").checkSanity());
     }
 
     /**
@@ -199,7 +201,7 @@ public class CronTabTest {
 
     @Test
     public void testHash1() throws Exception {
-        CronTab x = new CronTab("H H(5-8) * * *",new Hash() {
+        CronTab x = new CronTab("H H(5-8) H/3 H(1-10)/4 *",new Hash() {
             public int next(int n) {
                 return n-1;
             }
@@ -207,6 +209,8 @@ public class CronTabTest {
 
         assertEquals("59;", bitset(x.bits[0]));
         assertEquals("8;", bitset(x.bits[1]));
+        assertEquals("3;6;9;12;15;18;21;24;27;", bitset(x.bits[2]));
+        assertEquals("4;8;", bitset(x.bits[3]));
     }
 
     private static String bitset(long bits) {
@@ -221,7 +225,7 @@ public class CronTabTest {
 
     @Test
     public void testHash2() throws Exception {
-        CronTab x = new CronTab("H H(5-8) * * *",new Hash() {
+        CronTab x = new CronTab("H H(5-8) H/3 H(1-10)/4 *",new Hash() {
             public int next(int n) {
                 return 1;
             }
@@ -229,6 +233,8 @@ public class CronTabTest {
 
         assertEquals("1;", bitset(x.bits[0]));
         assertEquals("6;", bitset(x.bits[1]));
+        assertEquals("2;5;8;11;14;17;20;23;26;", bitset(x.bits[2]));
+        assertEquals("2;6;10;", bitset(x.bits[3]));
     }
 
     @Test public void hashedMinute() throws Exception {
@@ -240,4 +246,20 @@ public class CronTabTest {
         compare(new GregorianCalendar(2013, 2, 22, 13, 56), new CronTab("H H(12-13) * * *", Hash.from("stuff")).ceil(t));
     }
 
+    @Test public void hashSkips() throws Exception {
+        compare(new GregorianCalendar(2013, 2, 21, 16, 26), new CronTab("H/15 * * * *", Hash.from("stuff")).ceil(new GregorianCalendar(2013, 2, 21, 16, 21)));
+        compare(new GregorianCalendar(2013, 2, 21, 16, 41), new CronTab("H/15 * * * *", Hash.from("stuff")).ceil(new GregorianCalendar(2013, 2, 21, 16, 31)));
+        compare(new GregorianCalendar(2013, 2, 21, 16, 56), new CronTab("H/15 * * * *", Hash.from("stuff")).ceil(new GregorianCalendar(2013, 2, 21, 16, 42)));
+        compare(new GregorianCalendar(2013, 2, 21, 17, 11), new CronTab("H/15 * * * *", Hash.from("stuff")).ceil(new GregorianCalendar(2013, 2, 21, 16, 59)));
+        compare(new GregorianCalendar(2013, 2, 21, 0, 2), new CronTab("H(0-15)/3 * * * *", Hash.from("junk")).ceil(new GregorianCalendar(2013, 2, 21, 0, 0)));
+        compare(new GregorianCalendar(2013, 2, 21, 0, 2), new CronTab("H(0-3)/4 * * * *", Hash.from("junk")).ceil(new GregorianCalendar(2013, 2, 21, 0, 0)));
+        compare(new GregorianCalendar(2013, 2, 21, 1, 2), new CronTab("H(0-3)/4 * * * *", Hash.from("junk")).ceil(new GregorianCalendar(2013, 2, 21, 0, 5)));
+        try {
+            compare(new GregorianCalendar(2013, 2, 21, 0, 0), new CronTab("H(0-3)/15 * * * *", Hash.from("junk")).ceil(new GregorianCalendar(2013, 2, 21, 0, 0)));
+            fail();
+        } catch (ANTLRException x) {
+            // good
+        }
+    }
+
 }