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

Use channel thread to enqueue commands #617

Closed
mp911de opened this issue Oct 4, 2017 · 7 comments
Closed

Use channel thread to enqueue commands #617

mp911de opened this issue Oct 4, 2017 · 7 comments
Labels
type: feature A new feature
Milestone

Comments

@mp911de
Copy link
Collaborator

mp911de commented Oct 4, 2017

Using the channel thread to write the command makes a lot of sense because we could probably get rid of some synchronization code and subsequent channel I/O does not require further context switching. It would also unblock writes because we don't require synchronization to perform the actual channel write.

@nikolayk812
Copy link

nikolayk812 commented Oct 5, 2017

Too me it is more a like bug. I want invoke Lettuce CompletableFuture API and not to worry of caller thread being locked or catch an exception.

Is thrown in caller thread even though CompleteableFuture API is used.

com.lambdaworks.redis.RedisException: Request queue size exceeded: 500. Commands are not accepted until the queue size drops.

@mp911de
Copy link
Collaborator Author

mp911de commented Oct 5, 2017

Any validation happens beforehand to fail fast. It does not make sense to accept a command and reject it later if the validation can happen early.

@nikolayk812
Copy link

Yes, fail-fast is good, but it should be wrapped in RedisFuture, then I handle it in CompletableFuture.

@nikolayk812
Copy link

nikolayk812 commented Oct 5, 2017

Otherwise I have to handle exceptions in CompletableFuture and in caller thread. And getting exception in caller thread is counterintuitive.

@mp911de
Copy link
Collaborator Author

mp911de commented Oct 5, 2017

There's a point in handling failures inside the callbacks. Thanks a lot for your feedback.

I'd argue that you nevertheless need to handle errors in your call chain anyway – exceptions can happen due to other reasons before the actual command was constructed and dispatched (e.g. argument validation, infrastructure failures).

@nikolayk812
Copy link

As API user I would never prefer to get exceptions in caller thread.

All other exceptions could be caught or wrapped by the API implementation and return as CompletableFuture.

@mp911de mp911de modified the milestones: Lettuce 4.5.0, Icebox Jun 21, 2018
@mp911de mp911de modified the milestones: Icebox, 6.0 M1 Feb 21, 2020
@mp911de
Copy link
Collaborator Author

mp911de commented Feb 21, 2020

We refactored how exception signals are propagated into RedisCommand objects if a command is not eligible for being written to Redis. Previously, we relied on throws. As of Lettuce 6, we're propagating the failure via completeExceptionally(…) so the calls to async/reactive API's no longer need to care about handling exceptions with a separate approach.

@mp911de mp911de closed this as completed Feb 21, 2020
mp911de added a commit that referenced this issue Feb 21, 2020
…ures #617

Exceptions that were used to signal a full queue or prohibited commands are now no longer thrown but used to complete a command exceptionally. This approach friendlier to users of asynchronous/reactive APIs as the command outcome aligns with the programming model for error handling.
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

No branches or pull requests

2 participants