jsr107 / jsr107spec

JSR107 Cache Specification
Apache License 2.0
414 stars 164 forks source link

Refine Eternal Duration #356

Closed jerrinot closed 8 years ago

jerrinot commented 8 years ago

Contract of Duration says: @param durationAmount how long, in the specified units, the cache entries should live. 0 means eternal.

However 0 is interpreted as eternal only and if only the timeUnit is set to null. This should be reflected in the JavaDoc. My initial interpretation was "When durationAmount is 0 then the Duration is always eternal." I only realized I was wrong when I saw this method

cruftex commented 8 years ago

Also needs refining:

@throws NullPointerException if timeUnit is null

cruftex commented 8 years ago

Looking into the hazelcast defect, it is also interesting that this slipped through the TCK.

cruftex commented 8 years ago

A documentation for the special case in the other constructor should be added, too:

  public Duration(long startTime, long endTime) {
    if (startTime == Long.MAX_VALUE || endTime == Long.MAX_VALUE) {
      //we're dealing with arithmetic involving an ETERNAL value
      //so the result must be ETERNAL
      timeUnit = null;
      durationAmount = 0;
      . . .

Eternal is quite ambiguous.

Another finding:

  /**
   * Calculates the adjusted time (from the Epoc) given a specified time
   * (to be adjusted) by the duration.
   *
   * @param time the time from which to adjust given the duration
   * @return the adjusted time
   */
  public long getAdjustedTime(long time) {
    if (isEternal()) {
      return Long.MAX_VALUE;
    } else {
      return time + timeUnit.toMillis(durationAmount);
    }
  }
  1. special case is not documented
  2. time is in millis should be added
jerrinot commented 8 years ago

The TCK is right.

It turns out it was not a Hazelcast bug after all. Our implementation is correct, it's just the Duration contract which is hard to interpret and it confused the original reporter.

cruftex commented 8 years ago

Another finding:

Duration is immutable but not declared final. We cannot just declare it final, since this is not strictly API compatible.

Adding to the class comment:

Although this class is not declared final, it is not intended for extension. The behavior is undefined when subclasses are created and used.

I think that is the best we can do.