Guava: Add overloaded expireAfterWrite(), refreshAfterWrite() and expireAfterAccess() methods to CacheBuilder class

[Shenker93]

According to p.1 "API changes require discussion, use cases, etc. Code comes later.", I'm going to start discussion about benefits of proposed changes.

Now com.google.common.cache.CacheBuilder contains such methods as
refreshAfterWrite(long duration, TimeUnit unit)
expireAfterWrite(long duration, TimeUnit unit)
expireAfterAccess(long duration, TimeUnit unit)
Parameters of methods listed below are used to define durations in time.
However, in Java 8 new class, java.time.Duration, was presented. It can be used to save info about time durations in exactly one object. That is easier and more transparent in some real-world scenarios.

The idea is to add overloaded methods:
refreshAfterWrite(Duration duration) expireAfterWrite(Duration duration) expireAfterAccess(Duration duration)

Proposed changes provide possibilities to simplify code:
expireAfterWrite(Duration.ofHours(3)) // instead of expireAfterWrite(3, TimeUnit.HOURS)
It makes easier to declare constant durations too:

public static final Duration EXPIRES_IN = Duration.ofHours(1); // some code after statement expireAfterWrite(EXPIRES_IN)
instead of
public static final long DURATION_LENGTH = 1; public static final TimeUnit DURATION_TIME_UNIT = TimeUnit.HOURS; // some code after statement expireAfterWrite(DURATION_LENGTH, DURATION_TIME_UNIT)

Also, it allows to execute some arifmetic operations like addition or subtraction, that can simplify program logic if multiple (and dependant) durations in different methods and/or in different caches are used. Just something like this (simplified synthetic example):
Duration expiresAfterAccessIn = Duration.ofMinutes(30); CacheBuilder.newBuilder() .expireAfterAccess(expiresAfterAccessIn) .expireAfterWrite(expiresAfterAccessIn.plusHours(1)) .build();

This improvements do not lead to changing min java sdk version and do not break existing API.

[kluever]

EDIT: the response below was to the (now deleted?) suggestion that we add a bare long overload:

If we would agree on e.g. expireAfterWrite(long) time unit, it would also be a one-arg method with calls like expireAfterWrite(TimeUnit.SECONDS.toMillis(60)). Maybe that's a better way to go?

We strongly discourage the use of primitives (long, int, double, etc) for representing quantities of time. It's just too easy to screw up the units (I've seen literally hundreds of bugs in google of this nature).

Yes, Duration does sound like a good parameter choice here, but that would mean yet another fork between the Java7 and the Java8 branches of Guava.

There's also the consideration that we almost definitely will _not_ be retrofitting the util.concurrent APIs with Duration-accepting overloads (instead of the (TimeUnit, long) pairs), so maybe we shouldn't do so here either?

[fdesu]

@kluever yes, it was a bad suggestion to use long, as it will be very easy to mess things up, so I deleted it.

[kluever]

There's also the elephant in the room that we've more or less stopped maintaining common.cache in favor of @ben-manes 's excellent Caffeine cache.

I'd encourage you to store your expiration durations as java.time.Duration instances, and then when it comes time to call the CacheBuilder API, just convert. E.g.,

private static final Duration EXPIRES_IN = Duration.ofHours(1);
...
cacheBuilder.expireAfterWrite(EXPIRES_IN.toNanos(), NANOSECONDS)

toNanos() can overflow, but I'd be shocked if you have a cache with expirations > 292 years :-)

[kluever]

@ben-manes Can you expand a bit on your thumbs down vote?

[ben-manes]

I don’t think the new methods would provide much value, as you showed with
your last example. It could also be argued that Duration is less readable
when inlined. The addition would be strictly stylistic and the current
matches j.u.c. if consistency matters.

On Mon, Dec 4, 2017 at 8:37 AM Kurt Alfred Kluever [e-mail]
wrote:

There's also the elephant in the room that we've more or less stopped
maintaining common.cache in favor of @ben-manes
https://github.com/ben-manes 's excellent Caffeine
https://github.com/ben-manes/caffeine cache.

I'd encourage you to store your expiration durations as java.time.Duration
instances, and then when it comes time to call the CacheBuilder API, just
convert. E.g.,

private static final Duration EXPIRES_IN = Duration.ofHours(1);
...
cacheBuilder.expireAfterWrite(EXPIRES_IN.toNanos(), NANOSECONDS)

toNanos() can overflow, but I'd be shocked if you have a cache with
expirations > 292 years :-)

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/google/guava/issues/2999#issuecomment-349019362, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAXG9uihnYBMBXvRWJGbAFmL3nsVMMU0ks5s9B_jgaJpZM4QtfVu
.

[kluever]

@Shenker93 I'm going to close this given the discussions and work-arounds above.

[ben-manes]

Thanks to @kluever, we've added these overloads to Caffeine (upcoming release). Thanks Kurt!

[kluever]

@kevinb9n just said, "I think we won't mind adding the Duration overloads [to common.cache]"

[kluever]

Even more reason to add these overloads...I just discovered many folks inside of google writing things like:
cacheBuilder.expireAfterWrite(jodaDuration.getStandardHours(), TimeUnit.HOURS)

We're not going to add Joda Duration overloads, but the problem here is that the API silently truncates to the hour boundary. So if jodaDuration = Duration.standardMintues(59), then this expiration is set to 0!

The java.time.Duration API is a little better, in that it makes it more explicit that you're converting (thanks to the to prefix), but this is still very bug prone:
cacheBuilder.expireAfterWrite(javaDuration.toHours(), TimeUnit.HOURS)