Skip to content

DATAREDIS-779 ReactiveValueOperations.set[ifPresent|ifAbsent](…) does not return a value if value was not set #317

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

Closed
wants to merge 2 commits into from

Conversation

SystemOutPrint
Copy link
Contributor

No description provided.

@mp911de
Copy link
Member

mp911de commented Mar 6, 2018

Thanks for a pull request. We should discuss the actual issue and the solution within the related ticket first before applying changes.

This behavior is caused by Lettuce's suppression of null values and the change addresses only a single method without considering the other ones.

@SystemOutPrint
Copy link
Contributor Author

SystemOutPrint commented Mar 6, 2018

I want to use the setIfAbsent method to implement a distributed lock. The return value of setIfAbsent method is a Mono type data, I think the method will returns Mono.just(true) if the method succeeds, Otherwise Mono.just(false). But it's not so, Not quite sure why just emit the onComplete event when the setIfAbsent method failed.

@SystemOutPrint
Copy link
Contributor Author

SystemOutPrint commented Mar 6, 2018

   public void lock(String key) {
	Retry<Boolean> retry = Retry
			.<Boolean>onlyIf(rc -> rc.exception() instanceof LockHoldedException)
			.randomBackoff(Duration.ofSeconds(1), Duration.ofSeconds(2))
			.retryMax(MAX_NUM_OF_RETRY);

	template.opsForValue()
				.setIfAbsent(key, "1")
				.switchIfEmpty(Mono.error(new LockHoldedException()))
				.retryWhen(retry);
   }

@mp911de
Copy link
Member

mp911de commented Mar 6, 2018

This needs to be fixed in LettuceReactiveStringCommands.set(…). Do you want to apply the fix yourself or should I close this pull request and we do the fix?

@SystemOutPrint
Copy link
Contributor Author

SystemOutPrint commented Mar 7, 2018

I'll fix it.

@SystemOutPrint SystemOutPrint changed the title DATAREDIS-779 The reactive setIfAbsent method to modify the failure o… DATAREDIS-779 ReactiveValueOperations.set[ifPresent|ifAbsent](…) does not return a value if value was not set Mar 7, 2018
@mp911de
Copy link
Member

mp911de commented Mar 7, 2018

Thanks a lot. I'll take the pull request from here. We will apply the change only to Lovelace since that's a breaking change. Users of Spring Data Redis 2.0 currently don't expect an element to be emitted and after the change, we will emit elements.

mp911de pushed a commit that referenced this pull request Mar 7, 2018
…t(…) does not set a value.

We now emit false when SET XX/SET NX complete without setting the value because the condition was evaluated to not set the value. Previously, we did not emit a value as Redis responds null (nil). This behavior becomes visible through ReactiveValueOperations.set[ifPresent|ifAbsent](…) methods.

This is a breaking change in the sense that changes the character of ReactiveValueOperations.set[ifPresent|ifAbsent](…) from an optionally emitted element to always emit. Reactive sequences that currently resume operation with .switchIfEmpty(…) will no longer utilize the fallback publisher.

Original pull request: #317.
mp911de added a commit that referenced this pull request Mar 7, 2018
Reformat code. Add ticket references to tests.

Original pull request: #317.
@mp911de
Copy link
Member

mp911de commented Mar 7, 2018

That's now merged and polished.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants