Skip to content

Conversation

underwoo16
Copy link

@underwoo16 underwoo16 commented Sep 18, 2025

This would allow middlewares to instrument calls and retries with more granularity, as well as determine if raised errors are final or if they will be retried.

There would still need to be changes coordinated in DataDog/dd-trace-rb#3820 to close the initial issue (#119) but this is a generic approach that would also benefit anyone writing their own middlewares

@underwoo16 underwoo16 marked this pull request as ready for review September 18, 2025 20:29
@byroot
Copy link
Member

byroot commented Sep 18, 2025

So I'm favorable to this change, the issue however is that it's not really backward compatible.

So I need to think about it.

@byroot
Copy link
Member

byroot commented Sep 18, 2025

Random thoughts:

  • retry_attempt isn't that useful by itself, you got to know how many attempts total are allowed etc. It's not the most ergonomic.
  • A way to provide the info that wouldn't break the interface would be to have the info of whether the error will be retried in the exception itself.

@underwoo16
Copy link
Author

A way to provide the info that wouldn't break the interface would be to have the info of whether the error will be retried in the exception itself.

That would be a clean interface, I will take a look at this approach. It may be difficult since I think middleware sees exceptions before ensure_connected which handles the retries.


Another idea: How opposed are we to providing retry info via the config object which is already available in middleware calls? It handles a bit of retry logic https://github.com/redis-rb/redis-client/blob/master/lib/redis_client/config.rb#L143 - but I could also understand not wanting to add transient state to the config

byroot added a commit that referenced this pull request Sep 19, 2025
…lewares.

Fix: #254
Fix: #119
Ref: #119

Middlewares witness all network errors, but currently have no way of
knowing whether the error is final or is about to be retried.

In many case, you do want to distinguish the two because a low number
of transcient network errors can be expected.
@byroot
Copy link
Member

byroot commented Sep 19, 2025

I opened #255 as an alternative solution, I'd love to hear your feedback about it.

@underwoo16
Copy link
Author

Tracking retry_attempt on the connection is clean and I like the error.final? interface 👍🏼 Thanks for taking a look, I'll close this in favor of #255, left a couple questions over tehre

@underwoo16 underwoo16 closed this Sep 19, 2025
byroot added a commit that referenced this pull request Sep 20, 2025
…lewares.

Fix: #254
Fix: #119
Ref: #119

Middlewares witness all network errors, but currently have no way of
knowing whether the error is final or is about to be retried.

In many case, you do want to distinguish the two because a low number
of transcient network errors can be expected.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants