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

Add sample examples to test redis.io build #3051

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Add sample examples to test redis.io build #3051

wants to merge 6 commits into from

Conversation

uglide
Copy link
Collaborator

@uglide uglide commented Nov 13, 2024

Make sure that:

  • You have read the contribution guidelines.
  • You have created a feature request first to discuss your contribution intent. Please reference the feature request ticket number in the pull request.
  • 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.

@tishun
Copy link
Collaborator

tishun commented Nov 15, 2024

@uglide you need to run mvn formatter:format

@github-actions github-actions bot added type: dependency-upgrade A dependency upgrade type: documentation A documentation update type: task A general task labels Nov 22, 2024
@uglide uglide removed the type: dependency-upgrade A dependency upgrade label Nov 22, 2024
@github-actions github-actions bot added the type: dependency-upgrade A dependency upgrade label Nov 22, 2024
@uglide uglide removed the type: dependency-upgrade A dependency upgrade label Nov 22, 2024
@github-actions github-actions bot added the type: dependency-upgrade A dependency upgrade label Nov 22, 2024
@uglide uglide requested a review from tishun November 22, 2024 15:04
assertThat(res).isEqualTo("Deimos");
// REMOVE_END
System.out.println(res); // Deimos
}).then();
Copy link
Contributor

@ggivo ggivo Nov 26, 2024

Choose a reason for hiding this comment

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

Since we are demonstrating set and then get, removing the then() and returning the actual result from the get operation is better. This should be closer to the actual use.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above I would expect that we call block() here.

@ggivo is that what you mean by "return the actual result"?

Copy link
Contributor

@ggivo ggivo Nov 29, 2024

Choose a reason for hiding this comment

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

What I meant is that then() will discard the result from get() and will get as result Mono<Void> instead Mono<String>
I would expect that if anyone is following, a reactive example will be aware of .block() or ".subscribe()'
and hopefully, he will be searching for a way to integrate it within existing reactive code and know how to use the resulting Mono.
So the example code should be something like

            Mono<String> setAndGet = reactiveCommands.set("bike:1", "Deimos")
	                    .doOnNext(res-> System.out.println(res)) // OK
	                    .flatMap(v -> reactiveCommands.get("bike:1"))
	                    .doOnNext(res-> System.out.println(res)); // "Deimos"

assertThat(res).isEqualTo("OK");
// REMOVE_END
System.out.println(res); // OK
}).then();
Copy link
Contributor

Choose a reason for hiding this comment

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

same here. we can remove .then()

Comment on lines +29 to +41
// STEP_START set_get
Mono<Void> setAndGet = reactiveCommands.set("bike:1", "Deimos").doOnNext(v -> {
System.out.println(v); // OK
// REMOVE_START
assertThat(v).isEqualTo("OK");
// REMOVE_END
}).flatMap(v -> reactiveCommands.get("bike:1")).doOnNext(res -> {
// REMOVE_START
assertThat(res).isEqualTo("Deimos");
// REMOVE_END
System.out.println(res); // Deimos
}).then();
// STEP_END
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// STEP_START set_get
Mono<Void> setAndGet = reactiveCommands.set("bike:1", "Deimos").doOnNext(v -> {
System.out.println(v); // OK
// REMOVE_START
assertThat(v).isEqualTo("OK");
// REMOVE_END
}).flatMap(v -> reactiveCommands.get("bike:1")).doOnNext(res -> {
// REMOVE_START
assertThat(res).isEqualTo("Deimos");
// REMOVE_END
System.out.println(res); // Deimos
}).then();
// STEP_END
// STEP_START set_get
// Deimos
Mono<String> setAndGet = reactiveCommands.set("bike:1", "Deimos")
.doOnNext(res-> System.out.println(res)) // OK
.flatMap(v -> reactiveCommands.get("bike:1"))
.doOnNext(res-> System.out.println(res)); // Deimos
// REMOVE_START
String[] output = SysOutCaptor.CaptureSystemOut(() ->
StepVerifier.create(setAndGet)
.assertNext(v -> assertThat(v).isEqualTo("Deimos"))
.expectComplete()
.verify()
);
assertThat(output).containsSequence("OK", "Deimos");
// REMOVE_END
// STEP_END

I suggest extracting checks for code examples, as a separate step for readability and ease of maintenance

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.

Partial review, before your next iteration

@@ -0,0 +1,127 @@
// EXAMPLE: set_tutorial
package io.redis.examples.async;
Copy link
Collaborator

Choose a reason for hiding this comment

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

btw is this package a requirement?

we already have some examples in the io.lettuce.examples package inside src/test/java

@Test
// REMOVE_END
public void run() {
RedisClient redisClient = RedisClient.create("redis://localhost:6379");
Copy link
Collaborator

Choose a reason for hiding this comment

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

FWIW I would encourage our users to use the RedisUri pattern instead of providing the URL and port inside a string. Not critical, but I think for educational purposes it would be nice to have them done this way.

})
// REMOVE_END
.thenAccept(System.out::println) // OK
.toCompletableFuture();
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not necessarily bad, but you are not calling any of the methods that would guarantee that the main thread would not end before the executor threads.

As far as my understanding of how CompletableFuture works the code inside the stages / futures is executed by some thread pool (if no executor is provided then that would be the ForkJoinPool#commonPool) and without calling get() or join() for example theoretically the main thread might end before the thread that executes the future does. IMHO it would be good to implement some sort of synchronization here, simplest would be to call get(), but you can also do some sort of awat-all for all the futures in the end of the main method

assertThat(res).isEqualTo("Deimos");
// REMOVE_END
System.out.println(res); // Deimos
}).then();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above I would expect that we call block() here.

@ggivo is that what you mean by "return the actual result"?

@tishun tishun removed the type: dependency-upgrade A dependency upgrade label Dec 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: documentation A documentation update type: task A general task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants