GH-3805: Change to select between spinLock and pub-sub for the lock method #3819

Meteorkor · 2022-06-06T10:48:50Z

Resolves #3805

It has been changed so that the user can choose between the spinLock method and the pub-sub method.

In the default, spinLock is set as the default so that there is no problem in the past version.

Change RedisLock to abstract class and leave common parts
Separated into RedisSpinLock and RedisPubSubLock for each specialized part.

Any comments are always welcome.
thank you

…pub-sub method

artembilan

This is all great, but need some clarification.

Plus it would be great to add some documentation note with reasoning and links to reliable sources.

Thanks

...ration-redis/src/main/java/org/springframework/integration/redis/util/RedisLockRegistry.java

artembilan · 2022-06-06T14:49:18Z

...ration-redis/src/main/java/org/springframework/integration/redis/util/RedisLockRegistry.java

@@ -226,12 +200,27 @@ public void setCacheCapacity(int cacheCapacity) {
 		this.cacheCapacity = cacheCapacity;
 	}

+	/**
+	 * The default is {@link RedisLockType.SPIN_LOCK}<br>


Please, consider do not use <br> in JavaDocs.
Better to provide a <p> for a new paragraph.

I believe this method better to provide more info as "why?", what is the premise of such a decision?
Therefore end-user would know that in some cases the pub-sub won't work.

artembilan · 2022-06-06T14:50:02Z

...ration-redis/src/main/java/org/springframework/integration/redis/util/RedisLockRegistry.java

+	 * Select the lock method.
+	 *
+	 * @param redisLockType obtain RedisLockType
+	 * @since 6.0.0


We have to back-port the fix into 5.5.x, therefore @since 5.5.13

artembilan · 2022-06-06T14:51:49Z

...ration-redis/src/main/java/org/springframework/integration/redis/util/RedisLockRegistry.java

+	 * @since 6.0.0
+	 */
+	public void setRedisLockType(@Nonnull RedisLockType redisLockType) {
+		this.redisLockType = Objects.requireNonNull(redisLockType);


We prefer to use our own API: org.springframework.util.Assert.notNull(@Nullable Object object, String message).
However I don't think this method will need anything when we switch to plain boolean...

artembilan · 2022-06-06T14:54:51Z

...ration-redis/src/main/java/org/springframework/integration/redis/util/RedisLockRegistry.java

+		PUB_SUB_LOCK, SPIN_LOCK;
+	}
+
+	private Function<String, RedisLock> getRedisLockConstructor(@Nonnull RedisLockType redisLockType) {


We prefer our own org.springframework.lang.NonNull.
See its JavaDocs for a reason.

Although with the plain boolean flag I don't think we need any annotation at all....

artembilan · 2022-06-06T14:57:14Z

...ration-redis/src/main/java/org/springframework/integration/redis/util/RedisLockRegistry.java

+				"return false";
+		private final RedisScript<Boolean> obtainLockScript = new DefaultRedisScript<>(OBTAIN_LOCK_SCRIPT, Boolean.class);
+		private final RedisScript<Boolean> unLinkUnLockScript = new DefaultRedisScript<>(UNLINK_UNLOCK_SCRIPT, Boolean.class);
+		private final RedisScript<Boolean> deleteUnLockScript = new DefaultRedisScript<>(DELETE_UNLOCK_SCRIPT, Boolean.class);


It is better to have every class member to be separated with blank lines: much easier to read the code.

From here I even wonder if we need those constants for script texts.

Plus it looks like these RedisScript properties could be just static

...ration-redis/src/main/java/org/springframework/integration/redis/util/RedisLockRegistry.java

artembilan · 2022-06-06T15:01:17Z

...ration-redis/src/main/java/org/springframework/integration/redis/util/RedisLockRegistry.java

+				"  return true\n" +
+				"end\n" +
+				"return false";
+		private final RedisScript<Boolean> obtainLockScript = new DefaultRedisScript<>(OBTAIN_LOCK_SCRIPT, Boolean.class);


Isn't this property the same what we have in the RedisPubSubLock?
Cannot this go to the super class?

artembilan · 2022-06-06T15:02:29Z

...ration-redis/src/main/java/org/springframework/integration/redis/util/RedisLockRegistry.java

+			RedisLockRegistry.this.redisTemplate.delete(this.lockKey);
+		}
+
+		private boolean obtainLock() {


Isn't this method the same as in the RedisPubSubLock?
Cannot it go to the super class, too?

artembilan · 2022-06-06T15:04:30Z

...n-redis/src/test/java/org/springframework/integration/redis/util/RedisLockRegistryTests.java

@@ -779,5 +832,4 @@ private void waitForExpire(String key) throws Exception {
 	private static Map<String, Lock> getRedisLockRegistryLocks(RedisLockRegistry registry) {
 		return TestUtils.getPropertyValue(registry, "locks", Map.class);
 	}
-


As I said before: our preferences (for better code readability) is to have every single class member (even the last method) to be wrapped with blank lines: https://github.com/spring-projects/spring-integration/wiki/Spring-Integration-Framework-Code-Style#blank-lines

artembilan

I'm asking about docs one more time 😄

Thanks

artembilan · 2022-06-07T00:17:53Z

...ration-redis/src/main/java/org/springframework/integration/redis/util/RedisLockRegistry.java

+	private Function<String, RedisLock> getRedisLockConstructor(@NonNull RedisLockType redisLockType) {
+		return switch (redisLockType) {
+			case PUB_SUB_LOCK -> RedisPubSubLock::new;
+			case SPIN_LOCK -> RedisSpinLock::new;


We cannot use switch expression because we have to back-port to 5.5.x which is still based on Java 8.

artembilan · 2022-06-07T00:23:26Z

...ration-redis/src/main/java/org/springframework/integration/redis/util/RedisLockRegistry.java

+	 * @param redisLockType obtain RedisLockType
+	 * @since 5.5.13
+	 */
+	public void setRedisLockType(@NonNull RedisLockType redisLockType) {


We assume that everything is not null, unless it is marked as @Nullable.
So, we typically don't use this annotation at all, unless we declare a @NonNullApi in the package-info.java.
Then we need to revise all the public API to honor nullability contract, so Kotlin projects won't fail.

Since there is only this RedisLockRegistry in this org.springframework.integration.redis.util, i won't mind if you address this concern in this PR as well.

...ration-redis/src/main/java/org/springframework/integration/redis/util/RedisLockRegistry.java

artembilan · 2022-06-07T00:25:34Z

...ration-redis/src/main/java/org/springframework/integration/redis/util/RedisLockRegistry.java

@@ -260,9 +258,34 @@ public void destroy() {
 		}
 	}

-	private final class RedisLock implements Lock {
+	public enum RedisLockType {
+		PUB_SUB_LOCK, SPIN_LOCK;


These constants have to have JavaDocs: they are public API.

artembilan · 2022-06-07T00:27:01Z

...ration-redis/src/main/java/org/springframework/integration/redis/util/RedisLockRegistry.java

+				"end " +
+				"return false";
+
+		private static final RedisScript<Boolean> unLinkUnLockScript = new DefaultRedisScript<>(UNLINK_UNLOCK_SCRIPT, Boolean.class);


If it is static final, then it is a constant and therefore has to be declared only in capital letters.

artembilan · 2022-06-07T00:28:12Z

...ration-redis/src/main/java/org/springframework/integration/redis/util/RedisLockRegistry.java

 	}

+	/**
+	 * Used by RedisPubSubLock
+	 */
 	private static final class RedisUnLockNotifyMessageListener implements MessageListener {


This is private class - cannot it move into RedisPubSubLock class directly to preserve a context?

artembilan

I'd like to merge this, but there is need some documentation note to be added into redis.adoc in the [[redis-lock-registry]] section.

Let me know if you add it or you want me to do that for you!

Thank you.

artembilan · 2022-06-08T14:07:48Z

...ration-redis/src/main/java/org/springframework/integration/redis/util/RedisLockRegistry.java


-		private final String lockKey;
+	private Function<String, RedisLock> getRedisLockConstructor(@NonNull RedisLockType redisLockType) {


Remove, please, this @NonNull as we discussed before.

artembilan · 2022-06-08T14:44:54Z

Hi, @Meteorkor !

Thank you for prompt updates to the PR.

Let me know if you see my additional requests for docs like this:

I'd like to merge this, but there is need some documentation note to be added into redis.adoc in the [[redis-lock-registry]] section.

Meteorkor · 2022-06-08T14:46:32Z

redis.adoc

@artembilan
Oh thanks, could you please update it for me?

Thank you.

artembilan

Pulling this locally for final review and docs.

I will fix Checkstyle violation on merge:

Error: eckstyle] [ERROR] /home/runner/work/spring-integration/spring-integration/spring-integration-redis/src/main/java/org/springframework/integration/redis/util/RedisLockRegistry.java:54:8: Unused import - org.springframework.lang.NonNull. [UnusedImports]

artembilan · 2022-06-08T17:17:42Z

Merged as cf6ce96 and cherry-picked to 5.5.x

spring-projectsGH-3805: Change to select between spinLock method and …

1306c9c

…pub-sub method

artembilan requested changes Jun 6, 2022

View reviewed changes

spring-projectsGH-3805: fix javadoc, convention, lazy init

248b846

Meteorkor requested a review from artembilan June 6, 2022 23:25

artembilan requested changes Jun 7, 2022

View reviewed changes

spring-projectsGH-3805: fix javadoc, convention

c9bcce4

Meteorkor requested a review from artembilan June 7, 2022 22:37

artembilan requested changes Jun 8, 2022

View reviewed changes

spring-projectsGH-3805: Remove unnecessary annotations

26c466e

artembilan approved these changes Jun 8, 2022

View reviewed changes

artembilan closed this Jun 8, 2022


		private final String lockKey;
		private Function<String, RedisLock> getRedisLockConstructor(@NonNull RedisLockType redisLockType) {

GH-3805: Change to select between spinLock and pub-sub for the lock method #3819

GH-3805: Change to select between spinLock and pub-sub for the lock method #3819

Uh oh!

Conversation

Meteorkor commented Jun 6, 2022

Uh oh!

artembilan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

artembilan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

artembilan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

artembilan commented Jun 8, 2022

Uh oh!

Meteorkor commented Jun 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

artembilan left a comment

Choose a reason for hiding this comment

Uh oh!

artembilan commented Jun 8, 2022

Uh oh!

Uh oh!

Meteorkor commented Jun 8, 2022 •

edited

Loading