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

Kotlin Coroutine API #1387

Closed

Conversation

sokomishalov
Copy link
Collaborator

Related issue: #1386

@sokomishalov sokomishalov force-pushed the feature/kotlin-extensions branch 5 times, most recently from d373597 to 5d9993f Compare August 12, 2020 09:15
@sokomishalov sokomishalov changed the title WIP: kotlin API wrapper Kotlin API wrapper Aug 20, 2020
@mp911de
Copy link
Collaborator

mp911de commented Sep 4, 2020

Thank you for your pull request. This is an impressive contribution and we would like to include it.

For the implementation part, we typically keep all command implementations in a single support class to avoid an excessive number of classes.

I'm a bit confused why there are three variants, RedisSuspendableCommands.kt, RedisSuspendableAsyncCommands.kt, and RedisSuspendableReactiveCommands.kt. Having a singe suspendable API seems sufficient (RedisSuspendableCommands and segregated interfaces such as RedisSetSuspendableCommands, …).

I'm also missing the entry point, i.e. how to obtain suspendable commands from a Redis Connection. Finally, it would make also sense to provide a bit of documentation (simply drop an asciidoc file into src/main/asciidoc) to complete this PR.

@mp911de mp911de added status: waiting-for-feedback We need additional information before we can continue type: feature A new feature labels Sep 4, 2020
@sokomishalov sokomishalov force-pushed the feature/kotlin-extensions branch from 5d9993f to a898a40 Compare September 6, 2020 21:49
@sokomishalov sokomishalov marked this pull request as draft September 6, 2020 21:53
@sokomishalov
Copy link
Collaborator Author

sokomishalov commented Sep 6, 2020

I'm a bit confused why there are three variants, RedisSuspendableCommands.kt, RedisSuspendableAsyncCommands.kt, and RedisSuspendableReactiveCommands.kt. Having a singe suspendable API seems sufficient (RedisSuspendableCommands and segregated interfaces such as RedisSetSuspendableCommands, …).

My first idea was just to add extensions to async and reactive APIs (spring approach btw), but it is impossible now without changing a method signature/name/scope. Although suspend functions have a different signature in bytecode (by adding the Continuation parameter) kotlin compiler cannot distinguish it from a member function in the source code. It could be fixed by adding some prefix to extensions (i.e. aPing()), but it makes API much worse imho.
So I decided to propose the common "suspendable" interface, which is implemented by wrapping reactive and async operations (and who knows maybe will have some independent and "native" implementation in the future). I think implementation should be generated too because of its simpleness (see KotlinCompilationFactory).
That's my point, which is arguable and not ideal maybe.

For the implementation part, we typically keep all command implementations in a single support class to avoid an excessive number of classes.

it was just simpler to generate and then just have one class that implements all segregated interfaces by delegating to the generated ones (see RedisSuspendableReactiveCommandsImpl and RedisSuspendableAsyncCommandsImpl). I can make generated implementations internal to prevent exposing them and confusing end-users. But if it's inappropriate I'll rework the generator for this or just move it to a single one.

I'm also missing the entry point, i.e. how to obtain suspendable commands from a Redis Connection

Totally forgot, will add it soon
I think suspendable API is useless for plain java users so it could be provided in "kotlinese" way - extensions for reactive and async commands.

val cmd1 = connection.reactive().asSuspendable()
val cmd2 = connection.async().asSuspendable()

Finally, it would make also sense to provide a bit of documentation (simply drop an asciidoc file into src/main/asciidoc) to complete this PR.

Absolutely

@sokomishalov sokomishalov force-pushed the feature/kotlin-extensions branch 4 times, most recently from 85f8662 to 380de5f Compare September 7, 2020 13:00
@sokomishalov sokomishalov force-pushed the feature/kotlin-extensions branch from 380de5f to 3a129d1 Compare September 7, 2020 13:17
@mp911de mp911de added this to the 6.0.0 RC2 milestone Sep 15, 2020
@mp911de mp911de removed the status: waiting-for-feedback We need additional information before we can continue label Sep 15, 2020
@mp911de
Copy link
Collaborator

mp911de commented Sep 15, 2020

Thanks. I'm going to take this pull request from here targeting the 6.0 RC2 release. Specifically, I'll keep only a single suspendable implementation based on top of the reactive API to avoid a maintenance hell. There's no actual reason to keep both implementations.

@mp911de mp911de changed the title Kotlin API wrapper Kotlin Coroutine API Sep 15, 2020
mp911de pushed a commit that referenced this pull request Sep 15, 2020
mp911de added a commit that referenced this pull request Sep 15, 2020
Reduce Coroutine implementations to a single adapter using the reactive API. Rename implementations to Redis…Impl instead of Redis…ReactiveCommands. Remove comments from implementations as these are for internal use only.

Refactor type hierarchy. Expose a specific Cluster Coroutines API. Update method signatures to accept non-null values only. Fix nullability definitions for Sentinel and Transaction API.

Return Flow instead of List where applicable. Remove deprecated methods. Remove Coroutine access to the connection object to simplify future development. Remove suspend keyword for isOpen and other non-blocking methods. Remove StreamingChannel methods in favor of a Flow-based API.

Add integration tests. Add license headers.
@mp911de
Copy link
Collaborator

mp911de commented Sep 15, 2020

Thanks a lot for your contribution.

That's merged and polished now. I removed all deprecated methods and StreamingChannel methods from the API. It doesn't make sense to ship deprecated commands on a new API. Also, it makes sense to provide Flow-based variants instead of StreamingChannel as StreamingChannel comes from a time where we had no reactive API.

I created a few tickets to polish up the code generator. Feel free to have a look if you want to contribute to Lettuce.

@mp911de mp911de closed this Sep 15, 2020
@sokomishalov
Copy link
Collaborator Author

That's great!
Sure, thank you too.

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

Successfully merging this pull request may close these issues.

2 participants