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

feat: improve handling of LOADING response #657

Merged
merged 2 commits into from
Nov 2, 2024

Conversation

nesty92
Copy link
Contributor

@nesty92 nesty92 commented Oct 29, 2024

When a Redis node is loading data (e.g. after restart), it responds with LOADING errors. This makes the client more resilient during Redis node restarts and initial data loading phases.

Close: #656

@nesty92
Copy link
Contributor Author

nesty92 commented Oct 29, 2024

Hi @rueian, making some progress here and wanted to confirm that I'm heading in the right direction.

I noticed that Ping wasn't marked as read-only. Is there a specific reason for this, or might it just be an oversight?

I’ll wait for your feedback on this before continuing with the cluster client.

@rueian
Copy link
Collaborator

rueian commented Oct 29, 2024

Hi @nesty92,

I think we are on the right track, but I am concerned about marking PING as readonly. It is also not marked as readonly in the source https://github.com/redis/redis/blob/unstable/src/commands/ping.json.

Can we at least do that in a separate PR?

@nesty92
Copy link
Contributor Author

nesty92 commented Oct 30, 2024

I agree with your concern about marking PING as readonly. I will handle this in a separate PR to keep the changes isolated and easier to review.

@nesty92
Copy link
Contributor Author

nesty92 commented Oct 30, 2024

@rueian, I raised this question to get feedback from the Redis team regarding why the PING command is not marked as readonly

@nesty92 nesty92 force-pushed the feature/handle_loading_response branch from 0d2c239 to f69df5d Compare October 31, 2024 15:40
@nesty92 nesty92 marked this pull request as ready for review October 31, 2024 15:41
@nesty92
Copy link
Contributor Author

nesty92 commented Oct 31, 2024

Hi @rueian, I removed the changes for making PING read-only, but handling it in some way is important since PING should be the most reliable way to detect the loading state.

client.go Show resolved Hide resolved
client.go Show resolved Hide resolved
These changes make the client more resilient during Redis node restarts and initial
data loading phases by properly handling LOADING errors that occur when a Redis
node is loading data from persistence.

Signed-off-by: Ernesto Alejandro Santana Hidalgo <[email protected]>
@nesty92 nesty92 force-pushed the feature/handle_loading_response branch from f69df5d to 320021a Compare November 2, 2024 17:06
@rueian rueian merged commit 7c03866 into redis:main Nov 2, 2024
27 checks passed
@nesty92 nesty92 deleted the feature/handle_loading_response branch November 3, 2024 15:16
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.

Enhancement: Graceful Handling of Redis LOADING State in Rueidis
2 participants