Skip to content
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

DOC-4802 fix string example concurrency #3156

Merged

Conversation

andy-stark-redis
Copy link
Contributor

Make sure that:

  • You have read the contribution guidelines.
  • You applied code formatting rules using the mvn formatter:format target. Don’t submit any formatting related changes.
  • You submit test cases (unit or integration tests) that back your changes.

Copy link
Contributor

@ggivo ggivo left a comment

Choose a reason for hiding this comment

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

LGTM

Curious, what was the driver for replacing CompletableFuture.allOf() with individual joins.

@andy-stark-redis
Copy link
Contributor Author

LGTM

Curious, what was the driver for replacing CompletableFuture.allOf() with individual joins.

@ggivo Thanks for approving!

The issue with allOf() is that it executes the CompletableFuture instances concurrently, which was making the tests flaky. In the examples, we sometimes reuse keys between the individual code snippets and so a snippet running in parallel can sometimes change the value stored in the Redis DB before the assertion is called (so it gets the value from the wrong snippet and fails).

I consulted Tisho about this and we decided that using join() after each snippet is acceptable for now, but we'll wait to see what feedback we get from users. If they want to see more realistic examples of the async features in use then we might try another approach in the future.

@andy-stark-redis andy-stark-redis marked this pull request as ready for review February 3, 2025 15:47
@ggivo
Copy link
Contributor

ggivo commented Feb 3, 2025

@andy-stark-redis
Make sense. Thanks for getting back with the explanation.

Copy link
Collaborator

@tishun tishun left a comment

Choose a reason for hiding this comment

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

LGTM!

@tishun tishun merged commit c420392 into redis:main Feb 7, 2025
8 checks passed
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.

3 participants