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

Un-Deprecate io.lettuce.core.LettuceFutures #1453

Closed
andrewsensus opened this issue Oct 8, 2020 · 3 comments
Closed

Un-Deprecate io.lettuce.core.LettuceFutures #1453

andrewsensus opened this issue Oct 8, 2020 · 3 comments
Labels
type: task A general task
Milestone

Comments

@andrewsensus
Copy link
Contributor

Feature Request

LettuceFutures has several utility methods. They are even used in examples in the documentation. In the latest api documention, they are marked as deprecated with a note "since 6.0, use Futures instead". However, since Futures is "part of the internal API and may change without further notice", it doesn't seem appropriate for production use.

Is your feature request related to a problem? Please describe

I'd like a convenient way to await a redis future (from the async API) that behaves similarly to the sync API without a lot of boilerplate of catching and rewrapping exceptions. Previously LettuceFutures.awaitOrCancel provided that (and was also used internally in FutureSyncInvocationHandler).

Describe the solution you'd like

Un-Deprecate LettuceFutures.

It already forwards calls to internal.Futures. Perhaps that direction should be reversed, depending on which is considered more-core.

It would be nice for the internal implementation, FutureSyncInvocationHandler, to use the exact same methods that are exposed externally, but there might be tradoffs in dividing internal and external interfaces from each other as well as extra function call overhead.

Describe alternatives you've considered

Move io.lettuce.core.internal.Futures out of internal.

Positive: The implementation of FutureSyncInvocationHandler has changed to use Futures, so this would keep the external interface the same as the one used by the Sync API.

Negative: It might be more convenient only support the interface exposed by LettuceFutures, allowing additional methods on Futures to come-and-go without stability requirements imposed by external interfaces.

Teachability, Documentation, Adoption, Migration Strategy

The existing documentation still describes the methods -- only the deprecation notice would need removed.

@andrewsensus andrewsensus added the type: feature A new feature label Oct 8, 2020
@mp911de
Copy link
Collaborator

mp911de commented Oct 26, 2020

Thanks for making us aware of the issue. The actual idea was that the way of how futures get synchronized is an implementation detail that we don't want to expose in particular. That's why we moved all code from LettuceFutures into internal.Futures. We can include that change since LettuceFutures just forwards calls to Futures.

The direction of calls is already correct, however, the package structure we have already leads to cycles between io.lettuce.core and io.lettuce.core.internal as RedisFuture and the exceptions are basically a dependency magnet.

@mp911de mp911de added type: task A general task and removed type: feature A new feature labels Oct 26, 2020
@mp911de mp911de added this to the 6.0.1 milestone Oct 26, 2020
@mp911de
Copy link
Collaborator

mp911de commented Oct 26, 2020

Done.

@mp911de mp911de closed this as completed Oct 26, 2020
@andrewsensus
Copy link
Contributor Author

@mp911de Thanks!

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

No branches or pull requests

2 participants