Refine and clarify operations in the async cache implementation #2744

jxblum · 2023-10-17T22:33:41Z

mp911de

Did a first review pass.

mp911de · 2023-10-18T08:54:06Z

src/main/java/org/springframework/data/redis/cache/DefaultRedisCacheWriter.java

-			asyncCacheWriter = UnsupportedAsyncCacheWriter.INSTANCE;
-		}
+	private boolean isAsyncCacheSupportEnabled() {
+		return this.connectionFactory instanceof ReactiveRedisConnectionFactory;


We're testing assignability against an interface that defines potentially absent types such as Mono and Publisher. Loading interfaces can work under certain circumstances, however depending on the runtime verifier mechanics, loading ReactiveRedisConnectionFactory without Project Reactor can cause NoClassDefFoundError.

Therefore, we used a safe scheme that is being used across the majority of portfolio projects and there's no reason to deviate from that.

Just thinking...

Perhaps the class presence check should be on a Reactive type (e.g. Mono) then, given it does depend on the runtime verifier, particularly in strange cases where it can load the ReactiveConnectionFactory interface, which has no explicit Reactive types until you drill into ReactiveConnection. The real question is, is the ReactiveConnection as a return type on ReactiveConnectionFactory enough?

Also, by this same argument, then the only way a ReactiveConnectionFactory can exist is if Lettuce is on the classpath anyway, which has a hard dependency on Reactor. Of course, there is no guarantee that Lettuce will always pull in Reactor as a transitive dependency (hence this), but also the reason I argued for this (internal link).

I am convinced it should be both ReactiveConnectionFactory and a Reactive type like Mono or Flux.

mp911de · 2023-10-18T08:55:57Z

src/main/java/org/springframework/data/redis/cache/DefaultRedisCacheWriter.java

+		private Flux<Long> waitInterval(Duration period) {
+			return Flux.interval(Duration.ZERO, period);
+		}
+
+		private ByteBuffer toCacheLockKey(String cacheName) {
+			return ByteBuffer.wrap(createCacheLockKey(cacheName));
+		}
+
+		private ReactiveRedisConnectionFactory getReactiveConnectionFactory() {
+			return (ReactiveRedisConnectionFactory) DefaultRedisCacheWriter.this.connectionFactory;
+		}
+
+		private Mono<ReactiveRedisConnection> getReactiveConnection() {
+			return Mono.fromSupplier(getReactiveConnectionFactory()::getReactiveConnection);
+		}
+


These tiny methods are unnecessary and generate a lot of noise. Arguably, toCacheLockKey(String cacheName) can be an improvement.

I disagree. All of them are an improvement, and for very specific reasons.

They help reduce noise in the method where they are used. Otherwise "busy" code gets bunched together causing the code formatter to break lines at critical junctures.

Shorter methods are easier to read/understand, can be intelligently named, can be referred to if need be (not unlike code folding), and are easier to test in isolation.

Plus, these private methods can be reused, and in many places, they are.

In these methods...

waitForInterval(period) is more descriptive than Flux.interval(Duration.ZERO, period). What is Duration.ZERO anyway ("delay", I know, after looking it up)? Why do I even need to know it is Flux for that matter? It is an implementation detail. You don't know what Object.wait(..) does when you call it and you certainly wouldn't include its logic in the calling method.

getReactiveConnectionFactory() avoids the cast, could serve as an (internal) extension point if made package-private, and is more descriptive than connectionFactory alone (I am explicitly requesting a "Reactive" RedisConnectionFactory). Additionally, this method would allow me to add an assertion requiring a ReactiveRedisConnectionFactory and throwing an IllegalStateException otherwise, rather than clumsily getting a possible ClassCastException due to programmatical mistakes.

NOTE: In too many places, we refer to state variables directly rather than the appropriate getter (where applicable), preventing extensibility, something the Spring Framework excels at, and a strength I might add.

getReactiveConnection() ... Mono from a connectionFactory.getConnection(), Supplier. Need I say more? Having this logic spread all over the class is redundant and busy, creating extra work in methods that need Reactive connections.

What does it matter whether it is a variable or a method? Additional (helper) methods help remove clutter from methods and can be reused.

What was there before was certainly less readable.

mp911de · 2023-10-18T08:56:54Z

src/main/java/org/springframework/data/redis/cache/DefaultRedisCacheWriter.java

@@ -401,6 +382,14 @@ private static boolean shouldExpireWithin(@Nullable Duration ttl) {
 		return ttl != null && !ttl.isZero() && !ttl.isNegative();
 	}

+	private Expiration toExpiration(Duration ttl) {


These tiny methods are unnecessary and generate a lot of noise making it harder to read the code.

So this:

return connection.stringCommands().set(wrappedKey, wrappedValue, Expiration.from(ttl.toMillis(), TimeUnit.MILLISECONDS), SetOption.upsert());

Is easier to read than this:

stringCommands.set(wrappedKey, wrappedValue, toExpiration(ttl), SetOption.upsert())

Technically, I have even seen things likes this:

return connection.stringCommands().set(ByteBuffer.wrap(key), ByteBuffer.wrap(value), Expiration.from(ttl.toMillis(), TimeUnit.MILLISECONDS), SetOption.upsert());

Additionally, these methods can be use as Lambda's, such as in .map(:Function<S, T>) functions, which are useful in the context of Optionals, Streams, etc.

mp911de · 2023-10-18T08:58:08Z

src/main/java/org/springframework/data/redis/cache/DefaultRedisCacheWriter.java

-	 * @param cacheStatisticsCollector must not be {@literal null}.
-	 * @param batchStrategy must not be {@literal null}.
+	 * @param sleepTime sleep time between lock request attempts. Must not be {@literal null}.
+	 * Use {@link Duration#ZERO} to disable locking.


Please use the Spring Data formatter settings. There's no point in reformatting with a different formatter as the next committer is going to apply a different format again resulting in a lot of change noise.

I manually format Javadoc.

The Spring Data source code formatter for Javadoc needs to be fixed.

The excessive indentation is causing unnatural line breaks (such as sentences ending in and, or or, or While, etc, are bad writing style), creating orphans, and doing other inconsistent things that do not align with the core Spring Frameworks Javadoc guidelines.

mp911de · 2023-10-18T08:59:40Z

src/main/java/org/springframework/data/redis/cache/DefaultRedisCacheWriter.java

 	 */
 	DefaultRedisCacheWriter(RedisConnectionFactory connectionFactory, Duration sleepTime, BatchStrategy batchStrategy) {
 		this(connectionFactory, sleepTime, TtlFunction.persistent(), CacheStatisticsCollector.none(), batchStrategy);
 	}

 	/**
-	 * @param connectionFactory must not be {@literal null}.


What has happened to the rest of the Javadoc? GH shows that we only document sleepTime.

Only "must not be null" on @param tags is redundant and non-informative.

In general, I'd argue if we are not going to take the time to write meaningful Javadoc, then there is no point to have Javadoc on the method at all.

jxblum · 2023-10-18T21:12:17Z

Did a first review pass.

Thank you for the feedback. I reviewed each comment and cited my reasoning for the requested changes.

I reworked this PR to be more inline with our expectations.

Specifically, I:

Added a more robust check to the detection of Reactive types and usage, not unlike Spring Boot (here).
I reduced the number of private (helper) methods only slightly and reorganized them based on where they belong. Certain private methods remain because 1) they are reused 2) and/or are more descriptive (especially in the Reactive context).
Please! Can we follow the Spring Framework Javadoc guidelines.

Uses more descriptive names for operations, especially Reactive operations, by either calling a local, private method or introducing a (functional) variable with strongly typed parameters. Edits and refines Javadoc. Original Pull Request: spring-projects#2717 Closes spring-projects#2743

jxblum requested review from mp911de and christophstrobl October 17, 2023 22:33

jxblum added in: cache RedisCache and CacheManager type: enhancement A general enhancement labels Oct 17, 2023

jxblum added this to the 3.2 GA (2023.1.0) milestone Oct 17, 2023

jxblum force-pushed the issue/2743 branch from d270e17 to 298fc88 Compare October 17, 2023 22:37

jxblum force-pushed the main branch from 3679139 to d43adc9 Compare October 18, 2023 02:56

jxblum force-pushed the issue/2743 branch from 298fc88 to 406a294 Compare October 18, 2023 02:58

jxblum force-pushed the main branch from d43adc9 to a71f042 Compare October 18, 2023 03:07

Prepare issue branch for 2743.

9a81e6c

jxblum force-pushed the issue/2743 branch from 406a294 to df0c5c6 Compare October 18, 2023 03:13

mp911de requested changes Oct 18, 2023

View reviewed changes

jxblum force-pushed the issue/2743 branch from df0c5c6 to 2b5586f Compare October 18, 2023 21:00

jxblum force-pushed the issue/2743 branch from 2b5586f to 35df601 Compare October 18, 2023 21:13

jxblum changed the title ~~Refines and clarifies operations in the async cache implementation~~ Refine and clarify operations in the async cache implementation Nov 7, 2023

mp911de removed this from the 3.2 GA (2023.1.0) milestone Nov 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refine and clarify operations in the async cache implementation #2744

Refine and clarify operations in the async cache implementation #2744

jxblum commented Oct 17, 2023

mp911de left a comment

mp911de Oct 18, 2023

jxblum Oct 18, 2023

jxblum Oct 18, 2023 •

edited

Loading

jxblum Oct 18, 2023 •

edited

Loading

mp911de Oct 18, 2023

jxblum Oct 18, 2023 •

edited

Loading

jxblum Oct 18, 2023 •

edited

Loading

jxblum Oct 18, 2023

mp911de Oct 18, 2023

jxblum Oct 18, 2023 •

edited

Loading

jxblum Oct 18, 2023

jxblum Oct 18, 2023

mp911de Oct 18, 2023

jxblum Oct 18, 2023 •

edited

Loading

mp911de Oct 18, 2023

jxblum Oct 18, 2023 •

edited

Loading

jxblum commented Oct 18, 2023 •

edited

Loading

Refine and clarify operations in the async cache implementation #2744

Are you sure you want to change the base?

Refine and clarify operations in the async cache implementation #2744

Conversation

jxblum commented Oct 17, 2023

mp911de left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jxblum Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

jxblum Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jxblum Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

jxblum Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jxblum Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jxblum Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jxblum Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

jxblum commented Oct 18, 2023 • edited Loading

jxblum Oct 18, 2023 •

edited

Loading

jxblum Oct 18, 2023 •

edited

Loading

jxblum Oct 18, 2023 •

edited

Loading

jxblum Oct 18, 2023 •

edited

Loading

jxblum Oct 18, 2023 •

edited

Loading

jxblum Oct 18, 2023 •

edited

Loading

jxblum Oct 18, 2023 •

edited

Loading

jxblum commented Oct 18, 2023 •

edited

Loading