Caffeine: Consider adding `java.time.Duration` overloads to cache builder

[kluever]

I know you (Ben) thumbs down'ed the proposal for Guava's cache, but I'd be in favor of adding them to Caffeine.

In my opinion, java.time.Duration should be preferred over a pair of long, TimeUnit for all APIs going forward, with the exception of concurrency-only APIs (e.g., Futures and friends). To me, caching is different enough from those APIs that we should consider adding the overloads.

The long,TimeUnit pattern is error-prone and requires users pass around 2 values, one of which is a bare long.

[ben-manes]

Don't you think the builder readability is worse, e.g. Duration.ofHours(5) vs 5, TimeUnit.HOURS? As you had mentioned, you can pass around a duration and use duration.getSeconds() to convert, so there isn't much of a penalty. I would also use Duration in my application code for normal usages.

I don't see the drawback of the pair. What am I missing?

[kluever]

The drawback is that if you're passing the duration values around through various layers, you either need to pass a long,TimeUnit pair, or you need to pass a raw long and hope that people get the units correct.

If the durations are specified "in-line" with the cache builder, then sure, there's not much of a difference between the two.

[ben-manes]

Sorry, maybe I'm being obtuse. Wouldn't the user pass around java.time.Duration and then deconstruct to the pair only when building the cache? The raw long would be my penalty for internal code, but not exposed to the user.

[kluever]

Well if the user is passing around a java.time.Duration already, why make them decompose it at all? I'd rather write:
.expireAfterWrite(EXPIRE_AFTER)
than:
.expireAfterWrite(EXPIRE_AFTER.toNanos(), NANOSECONDS)
.expireAfterWrite(EXPIRE_AFTER.toMillis(), MILLISECONDS)
etc

[ben-manes]

Yep, so it adds a little convenience. Do you think that carries its weight?

In most of your usages, wouldn't you often externalize using the spec parser? Then you'd write expireAfterWrite = 1h in the configuration file. I thought their was a convenience parser for CacheBuilderSpec as a single variable for Google's binding (I've done similar using Typesafe Config for CaffeineSpec).

[kluever]

One other benefit, is that it would prevent folks like this from dropping sub-second precision (I saw 2 such instances of this in google3):

    .expireAfterAccess(ttl.getSeconds(), SECONDS)

Anyways, I don't think overloads add much "weight". The main reason we didn't add these in Guava is, well cache is basically in maintenance mode, and we also haven't forked it yet for Java8. Given that Caffeine is Java8+ and under active development, I'd be +1 on adding these.

[ben-manes]

Okay. I'm actually surprised because my recollection was the Guava team tended to dislike small overloads like this as not adding a significant benefit. Obviously if possible there were many refactorings to update usages to a newer convention, rather than allow rot. But I always got the sense that this was one of the API design differences from how Apache Commons approached things. When I reviewed against Bloch's rules, my thinking was to prefer being consistent with j.u.c, Guava, and that I hadn't observed a problem.

Did dropping sub-second precision have a negative effect. I know CacheBuilderSpec only allows down to seconds and expiring early shouldn't typically have much of an impact.

[kluever]

To be clear, I'm not speaking on behalf of the entire Guava team. If you're interested, I could ask other folks what they think. I do see the argument that this API really is a "concurrency API", but in my mind, this API is more of a user-facing configuration API, not a "low-level" concurrency API like j.u.c.

I don't _think_ dropping the sub-second precision had a negative effect in either case, but the point of Duration is so that users don't have to wonder about those types of questions to begin with.

Anyways, I don't feel super strongly here, so feel free to close this or leave it open without doing anything (and see if it gets any additional support).

[ben-manes]

I am certainly curious of what other opinions are.

I respect yours enough that I am inclined to add it.

[kevinb9n]

Kurt and I have embarked on an effort to stamp out all date-and-time-related bugs from our internal codebase. We're doing this by driving adoption of java.time, improving API designs, and adding a ton of static analysis to ban every usage pattern under the sun that we can't prove is safe.

For APIs that use either (long, TimeUnit) or (long ofFixedUnit), we keep finding an absolutely endless number of bugs - basic unit mismatch, arithmetic error, etc. Part of how we want to address this globally is to drive usage of Duration as much as we possibly can. Then it becomes only the few ways of creating durations that we have to spread static checks all over.

Given this, I'm more or less coming to the opinion that every API that wants to accept a logical duration should always have a Duration overload. And that it's generally a positive if that's the only overload it has. I see no reason not to make this recommendation for everything in java.util.concurrent -- obviously the (long, TimeUnit) would never go away, but I think more usage of Duration should be encouraged. And when a user does have a Duration I think it's a bad thing to make them decompose it.

[ben-manes]

Thank you both. I am surely not going against two opinions that I look up to.

[kevinb9n]

Please do go against our opinions when they're bad. :-)

[Stephan202]

Very nice discussion here. FWIW, in our company I also strongly advocate sticking to a small set of well-established java.time patterns:

• Always use Duration to denote logical durations (as discussed here).
• Always derive the current time from a provided (generally: dependency-injected) time source (generally a java.time.Clock) instead of using the various static java.time.SomeClass#now methods.
• Use Instant instead of (Zoned)DateTime when reasonably possible. (Push local or timezone-based computations to the "edges" (e.g. REST interfaces) of the application.)
• Don't pass a Clock to methods that are only concerned with the current time; just tell them the current time.
• ... etc.

@kevinb9n I hope the Google team open-sources the Error Prone/Refaster checks you're alluding to. :D

Back on topic: in our code base we also have a bunch of places where we decompose a Duration (either a private static final constant or some Spring property) as a (long, TimeUnit) pair. And indeed, some devs went for millisecond precision and others went for seconds. So +1 on this feature request.

[kevinb9n]

Getting off-topic, but just had to say: that's a great set of rules; we agree. In fact, we discourage ZDT (really anything where a time zone ambiguously 'stows away' inside a value) strongly enough that we made our own TimeSource class to use instead of Clock. It has just now() returning Instant, and asClock(ZoneId) for interoperation.

[ben-manes]

With regards to low-level inspections in cache.policy(), should methods use java.time instead? This would mean the current versions would be removed in v3.0.

interface Expiration<K, V> { // To be renamed FixedExpiration in version 3.0.0
  OptionalLong ageOf(K key, TimeUnit unit);
  long getExpiresAfter(TimeUnit unit);
  void setExpiresAfter(long duration, TimeUnit unit);
  Map<K, V> oldest(int limit);
  Map<K, V> youngest(int limit);
}

interface VarExpiration<K, V> {
  OptionalLong getExpiresAfter(K key, TimeUnit unit);
  void setExpiresAfter(@Nonnull K key, long duration, TimeUnit unit);
  boolean putIfAbsent(@Nonnull K key, V value, long duration, TimeUnit unit);
  void put(K key, V value, long duration, TimeUnit unit);
  Map<K, V> oldest(int limit);
  Map<K, V> youngest(int limit);
}

[kluever]

If the interfaces aren't public, then I think it's up to you which you think is easier to deal with for implementation purposes.

[ben-manes]

The interfaces are public (I shortened them for readability here). This hides all the nasty details from the Guava-style core interfaces, while also giving the functionality needed for some use-cases.

// Modify the cache's expiration time
cache.policy().expireAfterWrite().ifPresent(policy -> {
  policy.setExpiresAfter(1, TimeUnit.MINUTE);
});

[kluever]

Ahh, OK...well adding the overloads to the interfaces can probably be done safely w/ default methods, right? If so, I think the changes should be made there as well.

Long term, are your plans to eventually deprecate (& remove) the long, TimeUnit overloads or just leave them in place?

[ben-manes]

Yes, defaults would be used here. For now they would call the existing methods to adapt from.

Long term I'd prefer having only one style, since it adds a lot of noise, so I'd remove some in v3. I think you'd prefer to have the java.time style promoted, so I would use that if possible. Just wanted to confirm and I'll add them.

[kluever]

Yep, +1 on using Duration consistently throughout the entire API.

[kluever]

Thanks @ben-manes !

[ben-manes]

Released. Thanks!