-
Notifications
You must be signed in to change notification settings - Fork 979
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
base: main
Are you sure you want to change the base?
Conversation
@uglide you need to run |
Co-authored-by: Tihomir Mateev <[email protected]>
assertThat(res).isEqualTo("Deimos"); | ||
// REMOVE_END | ||
System.out.println(res); // Deimos | ||
}).then(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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"?
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
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()
// 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// 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
There was a problem hiding this 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; |
There was a problem hiding this comment.
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"); |
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
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"?
Make sure that:
mvn formatter:format
target. Don’t submit any formatting related changes.