Implement Duration.compareTo() (#582)

* Update Duration.compareTo()

* Add check for negative duration for interval

Author: songy23

api/src/main/java/io/opencensus/common/Duration.java

@@ -22,6 +22,7 @@
 import static io.opencensus.common.TimeUtil.NANOS_PER_MILLI;
 
 import com.google.auto.value.AutoValue;
+import com.google.common.primitives.Longs;
 import javax.annotation.concurrent.Immutable;
 
 /**
@@ -31,8 +32,7 @@
  */
 @Immutable
 @AutoValue
-// TODO(songya): implements Comparable<Duration>
-public abstract class Duration {
+public abstract class Duration implements Comparable<Duration> {
   private static final Duration ZERO = create(0, 0);
 
   /**
@@ -88,5 +88,22 @@ public static Duration fromMillis(long millis) {
    */
   public abstract int getNanos();
 
+  /**
+   * Compares this {@code Duration} to the specified {@code Duration}.
+   *
+   * @param otherDuration the other {@code Duration} to compare to, not {@code null}.
+   * @return the comparator value: zero if equal, negative if this duration is smaller
+   *     than otherDuration, positive if larger.
+   * @throws NullPointerException if otherDuration is {@code null}.
+   */
+  @Override
+  public int compareTo(Duration otherDuration) {
+    int cmp = Longs.compare(getSeconds(), otherDuration.getSeconds());
+    if (cmp != 0) {
+      return cmp;
+    }
+    return Longs.compare(getNanos(), otherDuration.getNanos());
+  }
+
   Duration() {}
 }

api/src/main/java/io/opencensus/common/TimeUtil.java

@@ -16,7 +16,7 @@
 
 package io.opencensus.common;
 
-/** Created by bdrutu on 6/14/17. */
+/** Util class for {@link Timestamp} and {@link Duration}. */
 final class TimeUtil {
   static final long MAX_SECONDS = 315576000000L;
   static final int MAX_NANOS = 999999999;

api/src/main/java/io/opencensus/common/Timestamp.java

@@ -24,6 +24,7 @@
 
 import com.google.auto.value.AutoValue;
 import com.google.common.math.LongMath;
+import com.google.common.primitives.Longs;
 import java.math.RoundingMode;
 import javax.annotation.concurrent.Immutable;
 
@@ -143,15 +144,11 @@ public Duration subtractTimestamp(Timestamp timestamp) {
    */
   @Override
   public int compareTo(Timestamp otherTimestamp) {
-    int cmp = compareLong(getSeconds(), otherTimestamp.getSeconds());
+    int cmp = Longs.compare(getSeconds(), otherTimestamp.getSeconds());
     if (cmp != 0) {
       return cmp;
     }
-    return compareLong(getNanos(), otherTimestamp.getNanos());
-  }
-
-  private static int compareLong(long x, long y) {
-    return (x < y) ? -1 : ((x == y) ? 0 : 1);
+    return Longs.compare(getNanos(), otherTimestamp.getNanos());
   }
 
   // Returns a Timestamp with the specified duration added.

api/src/test/java/io/opencensus/common/DurationTest.java

@@ -62,6 +62,21 @@ public void testDurationFromMillisNegative() {
     assertThat(Duration.fromMillis(-3456)).isEqualTo(Duration.create(-3, -456000000));
   }
 
+  @Test
+  public void duration_CompareLength() {
+    assertThat(Duration.create(0, 0).compareTo(Duration.create(0, 0))).isEqualTo(0);
+    assertThat(Duration.create(24, 42).compareTo(Duration.create(24, 42))).isEqualTo(0);
+    assertThat(Duration.create(-24, -42).compareTo(Duration.create(-24, -42))).isEqualTo(0);
+    assertThat(Duration.create(25, 42).compareTo(Duration.create(24, 42))).isEqualTo(1);
+    assertThat(Duration.create(24, 45).compareTo(Duration.create(24, 42))).isEqualTo(1);
+    assertThat(Duration.create(24, 42).compareTo(Duration.create(25, 42))).isEqualTo(-1);
+    assertThat(Duration.create(24, 42).compareTo(Duration.create(24, 45))).isEqualTo(-1);
+    assertThat(Duration.create(-24, -45).compareTo(Duration.create(-24, -42))).isEqualTo(-1);
+    assertThat(Duration.create(-24, -42).compareTo(Duration.create(-25, -42))).isEqualTo(1);
+    assertThat(Duration.create(24, 42).compareTo(Duration.create(-24, -42))).isEqualTo(1);
+
+  }
+
   @Test
   public void testDurationEqual() {
     // Positive tests.

core/src/main/java/io/opencensus/stats/View.java

@@ -195,6 +195,8 @@ public final <T> T match(
     @AutoValue
     public abstract static class Interval extends AggregationWindow {
 
+      private static final Duration ZERO = Duration.create(0, 0);
+
       Interval() {}
 
       /**
@@ -215,6 +217,7 @@ public abstract static class Interval extends AggregationWindow {
        * @return an interval {@code AggregationWindow}.
        */
       public static Interval create(Duration duration) {
+        checkArgument(duration.compareTo(ZERO) > 0, "Duration must be positive");
         return new AutoValue_View_AggregationWindow_Interval(duration);
       }
 

core/src/test/java/io/opencensus/stats/ViewTest.java

@@ -140,6 +140,11 @@ public void preventNullNameString() {
     View.Name.create(null);
   }
 
+  @Test(expected = IllegalArgumentException.class)
+  public void preventNegativeIntervalDuration() {
+    Interval.create(negativeTenSeconds);
+  }
+
   @Test
   public void testViewNameEquals() {
     new EqualsTester()
@@ -159,4 +164,5 @@ public void testViewNameEquals() {
   private final List<Aggregation> aggregations = Arrays.asList(Sum.create(), Count.create());
   private final Duration minute = Duration.create(60, 0);
   private final Duration twoMinutes = Duration.create(120, 0);
+  private final Duration negativeTenSeconds = Duration.create(-10, 0);
 }

core_impl/src/main/java/io/opencensus/implcore/stats/IntervalBucket.java

@@ -31,6 +31,8 @@
 /** The bucket with aggregated {@code MeasureValue}s used for {@code IntervalViewData}. */
 final class IntervalBucket {
 
+  private static final Duration ZERO = Duration.create(0, 0);
+
   private final Timestamp start;
   private final Duration duration;
   private final List<Aggregation> aggregations;
@@ -40,6 +42,7 @@ final class IntervalBucket {
   IntervalBucket(Timestamp start, Duration duration, List<Aggregation> aggregations) {
     checkNotNull(start, "Start");
     checkNotNull(duration, "Duration");
+    checkArgument(duration.compareTo(ZERO) > 0, "Duration must be positive");
     checkNotNull(aggregations, "Aggregations");
     this.start = start;
     this.duration = duration;
@@ -75,7 +78,7 @@ void record(List<TagValue> tagValues, double value) {
    */
   double getFraction(Timestamp now) {
     Duration elapsedTime = now.subtractTimestamp(start);
-    checkArgument(toMillis(elapsedTime) >= 0 && toMillis(elapsedTime) < toMillis(duration),
+    checkArgument(elapsedTime.compareTo(ZERO) >= 0 && elapsedTime.compareTo(duration) < 0,
         "This bucket must be current.");
     return ((double) toMillis(elapsedTime)) / toMillis(duration);
   }

core_impl/src/test/java/io/opencensus/implcore/stats/IntervalBucketTest.java

@@ -47,6 +47,7 @@ public class IntervalBucketTest {
 
   private static final double TOLERANCE = 1e-6;
   private static final Duration MINUTE = Duration.create(60, 0);
+  private static final Duration NEGATIVE_TEN_SEC = Duration.create(-10, 0);
   private static final Timestamp START = Timestamp.create(60, 0);
   private static final BucketBoundaries BUCKET_BOUNDARIES =
       BucketBoundaries.create(Arrays.asList(-10.0, 0.0, 10.0));
@@ -66,6 +67,12 @@ public void preventNullDuration() {
     new IntervalBucket(START, null, AGGREGATIONS_V1);
   }
 
+  @Test
+  public void preventNegativeDuration() {
+    thrown.expect(IllegalArgumentException.class);
+    new IntervalBucket(START, NEGATIVE_TEN_SEC, AGGREGATIONS_V1);
+  }
+
   @Test
   public void preventNullAggregationList() {
     thrown.expect(NullPointerException.class);

findbugs-exclude.xml

@@ -11,5 +11,11 @@
     <Class name="io.opencensus.common.Timestamp" />
     <Method name="compareTo" />
   </Match>
+  <Match>
+    <!-- Reason: Equal is implemented in the AutoValue generated class. -->
+    <Bug pattern="EQ_COMPARETO_USE_OBJECT_EQUALS"/>
+    <Class name="io.opencensus.common.Duration" />
+    <Method name="compareTo" />
+  </Match>
 
 </FindBugsFilter>