Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Add multiple reactive keys exist checker #2918

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

Copy link
Contributor

@AnneMayor AnneMayor commented May 26, 2024

  • You have read the Spring Data contribution guidelines.
  • You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
  • You submit test cases (unit or integration tests) that back your changes.
  • You added yourself as author in the headers of the classes you touched. Amend the date range in the Apache license header if needed. For new types, add the license header (copy from another file and set the current year only).

Resolved #2883

.verifyComplete();
assertThat(nativeCommands.exists(KEY_2)).isEqualTo(1L);
assertThat(nativeCommands.exists(KEY_1)).isEqualTo(0L);
assertThat(nativeCommands.exists(KEY_1)).isZero();
Copy link
Contributor Author

@AnneMayor AnneMayor May 26, 2024
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just changed the command to make better readability according to sonarlint rules.

mp911de reacted with thumbs up emoji
* @return {@link Mono} emitting {@literal true} if all keys exist.
* @see <a href="https://redis.io/commands/exists">Redis Documentation: EXISTS</a>
*/
default Mono<Boolean> exists(List<ByteBuffer> keys) {
Copy link
Contributor Author

@AnneMayor AnneMayor May 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created a validation method which is for checking if parameterized all keys exist in redis cache. If one of them does not exist, it returns false.
@kutlueren , please check if this function is what you meant to use.

Copy link

@kutlueren kutlueren May 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @AnneMayor

Awesome to see that you added the function which carries out what I suggested. It is actually more straight forward than I initially thought, maybe I got confused in the codebase but nice! I shall try it out as soon as it gets merged.

AnneMayor reacted with thumbs up emoji
Copy link
Member

@mp911de mp911de Sep 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This implementation calls EXISTS for each key issuing a command for each element in the list.

Redis' EXISTS command accepts multiple arguments and returns how many of the keys exist. Our interface should use the existing functionality instead of putting another layer on top of commands with a worse performance behavior.

The issue we have here is however that exists(Publisher<KeyCommand> keys) and exists(Publisher<List<KeyCommand>> keys) or exists(Publisher<MultiKeyCommand>> keys) erase to the same type and we cannot simply introduce such an override.

I suggest to implement Mono<Long> exists(List<ByteBuffer> keys) directly instead of using default methods. Also, ReactiveRedisOperations should be extended with countExistingKeys(...) similar to RedisTemplate.countExistingKeys(...).

AnneMayor reacted with thumbs up emoji
Copy link

Hey @AnneMayor, I just wanted to follow up on the topic :) Is this being planned to be merged soon?

Copy link
Contributor Author

Hey @kutlueren ,
Unfortunately I have no feedback from our maintainer🥲
I am still waiting for their feedback.

kutlueren reacted with thumbs up emoji

Copy link

Hey @kutlueren , Unfortunately I have no feedback from our maintainer🥲 I am still waiting for their feedback.

@mp911de would you mind taking a look at this one when you have time?

Copy link
Contributor Author

@mp911de ,
Please let me know if this PR has some issues🙏

Copy link
Member

@mp911de mp911de left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a review pass and left a comment regarding the design and implementation. Care to have a look?

AnneMayor reacted with thumbs up emoji
* @return {@link Mono} emitting {@literal true} if all keys exist.
* @see <a href="https://redis.io/commands/exists">Redis Documentation: EXISTS</a>
*/
default Mono<Boolean> exists(List<ByteBuffer> keys) {
Copy link
Member

@mp911de mp911de Sep 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This implementation calls EXISTS for each key issuing a command for each element in the list.

Redis' EXISTS command accepts multiple arguments and returns how many of the keys exist. Our interface should use the existing functionality instead of putting another layer on top of commands with a worse performance behavior.

The issue we have here is however that exists(Publisher<KeyCommand> keys) and exists(Publisher<List<KeyCommand>> keys) or exists(Publisher<MultiKeyCommand>> keys) erase to the same type and we cannot simply introduce such an override.

I suggest to implement Mono<Long> exists(List<ByteBuffer> keys) directly instead of using default methods. Also, ReactiveRedisOperations should be extended with countExistingKeys(...) similar to RedisTemplate.countExistingKeys(...).

AnneMayor reacted with thumbs up emoji
Copy link
Contributor Author

I did a review pass and left a comment regarding the design and implementation. Care to have a look?

Thanks a lot. I am going to apply your review within this weekend 👍

Copy link

dreamstar-enterprises commented Jan 15, 2025
edited
Loading

Any luck solving this?

I have the same issue. Wanting to check the existence of multiple keys using exists in a single network call, without resorting to Lua, but using Springs own implementation

Copy link

dreamstar-enterprises commented Jan 15, 2025
edited
Loading

My current workaround, since the spring API does not support exists with multiple keys yet.

/**
 * Executes expired session cleanup in Redis.
 *
 * Spring Session Redis uses multiple keys per session:
 * 1. Hash key (namespace:sessions:{id}) - Stores session data with 35 min TTL
 * 2. ZSet entry - Tracks session expiration times for batch cleanup
 *
 * Process flow:
 * 1. Query ZSet for sessions that should be expired
 * 2. Access their hash keys to trigger Redis expiration events
 * - Redis only guarantees expiration events when keys are accessed
 * - Spring Session listens for these events to cleanup related keys
 * 3. Remove processed entries from the ZSet
 *
 * Implementation details:
 * - Uses Lua script for efficient multi-key EXISTS checks
 * - Handles quoted session IDs in ZSet vs unquoted in hash keys
 * - Relies on Redis keyspace events for actual session deletion
 * - Bounded elastic scheduling for background processing
 *
 * @param context Operation boundaries and limits for ZSet query
 * @return Mono<Void> completing when cleanup process finishes
 * @throws RedisConnectionException if Redis operations fail
 *
 * @see "https://docs.spring.io/spring-session/reference/configuration/reactive-redis-indexed.html#how-spring-session-cleans-up-expired-sessions"
 */
 private fun cleanupExpiredSessions(context: CleanupContext): Mono<Void> =
 redisOperations.opsForZSet()
 .reverseRangeByScore(redisKeyExpirations, context.range, context.limit)
 .collectList()
 .flatMap { sessionIds ->
 if (sessionIds.isEmpty()) {
 logger.debug("No expired sessions found")
 Mono.empty()
 } else {
 logger.debug("Found {} expired sessions to remove", sessionIds.size)
 // Prepare session keys (remove quotes around session id in zset)
 val sessionKeys = sessionIds.map { sessionId ->
 "${sessionProperties.redis?.sessionNamespace}:sessions:${sessionId.trim('"')}"
 }.also { keys ->
 logger.debug("Checking Redis keys: {}", keys)
 }
 // First read the hash keys to trigger expiration event
 val script = """
 local keys = ARGV
 local count = 0
 for i, key in ipairs(keys) do
 count = count + redis.call('EXISTS', key)
 end
 return count
 """
 redisOperations.execute(
 RedisScript.of(script, Long::class.java),
 listOf(), // KEYS list is empty
 sessionKeys
 )
 .doOnNext { count ->
 logger.debug("Accessed {} session hash keys", count)
 }
 .flatMap { _ ->
 redisOperations.opsForZSet()
 .remove(redisKeyExpirations, *sessionIds.toTypedArray())
 .doOnSuccess { logger.debug("Successfully removed {} expired sessions", sessionIds.size) }
 .doOnError { e -> logger.error("Error during removal: ${e.message}") }
 }
 .then()
 }
 }
 .subscribeOn(Schedulers.boundedElastic())

Copy link
Contributor Author

@dreamstar-enterprises Oh, for real? I am gonna check. Sorry for the late response. Since I have some business, I ended up keeping going this PR again. Will review your issue and give some feedback.

Copy link
Contributor Author

Sorry for being late. I have lots of personal business and I am going to resolve this PR comments until this weekend. Please, keep that in mind.

@AnneMayor AnneMayor force-pushed the add-multiple-reactive-keys-exist-checker branch from 5b033a1 to 79ee7fd Compare March 8, 2025 13:10
Signed-off-by: Anne Lee <anne.lee@buzzvil.com>
@AnneMayor AnneMayor force-pushed the add-multiple-reactive-keys-exist-checker branch from 79ee7fd to 38637d8 Compare March 8, 2025 13:13
Copy link
Contributor Author

@mp911de could you review this PR again for me?
Also, I have a question. Do you think that it would be better if I add this exist method into class ReactiveRedisOperations?

mp911de pushed a commit that referenced this pull request Mar 12, 2025
Closes #2883
Original pull request: #2918
Signed-off-by: Anne Lee <anne.lee@buzzvil.com>
mp911de added a commit that referenced this pull request Mar 12, 2025
Reformat code, add since and author tags.
See #2883
Original pull request: #2918 
Copy link
Member

mp911de commented Mar 12, 2025

No worries, some pull requests remain open for quite some time because we have to prioritize sometimes other things and so sometimes, pull requests don't get much attention.

In any case, thank you for your contribution. That's merged and polished now.

AnneMayor reacted with thumbs up emoji AnneMayor reacted with heart emoji

@mp911de mp911de added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Mar 12, 2025
@mp911de mp911de added this to the 3.5 M2 (202500) milestone Mar 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mp911de mp911de Awaiting requested review from mp911de

1 more reviewer

@kutlueren kutlueren kutlueren left review comments

Reviewers whose approvals may not affect merge requirements
Labels
type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ReactiveKeyCommands.Exists to check multiple key existence

AltStyle によって変換されたページ (->オリジナル) /