Skip to content

Redis health indicator requests more information than it needs resulting in unnecessarily large responses from Redis #24208

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

Closed

Conversation

xJoeWoo
Copy link
Contributor

@xJoeWoo xJoeWoo commented Nov 19, 2020

Health check of Redis only uses redis_version field in server section, however, info command without any section is used so that all sections of information of Redis will be returned. The size of responded result can be up to 3.5KB for each query. If health check interval is set low like 2 seconds, decoding/processing the result may cost unnecessary CPU time.

image

This PR adds section argument server of info command for reducing sections to be queried.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Nov 19, 2020
@snicoll
Copy link
Member

snicoll commented Nov 19, 2020

Thanks for the PR.

Regardless of the outcome of this issue, a call to info should not lead to this so please consider reporting that against the Redisson project.

See also https://redis.io/commands/info

@snicoll snicoll added the for: team-attention An issue we'd like other members of the team to review label Nov 19, 2020
@snicoll
Copy link
Member

snicoll commented Nov 19, 2020

I find a little odd that we have to hardcode "server" in that method call there. Checking with @mp911de, this API was built to give maximum flexibility if Redis decided to change something. Mark is open to an update of the API to take also a Enum with the well-known command. Before proceeding, I'd like to see what the rest of the team thinks about hardcoding "server" there.

@wilkinsona
Copy link
Member

Given that server is documented as an input for the INFO command, hardcoding it seems fine to me. An enum of String constant would be an improvement, but in their absence I see no reason not to use the existing info(String) API with a hard-coded server argument.

@xJoeWoo
Copy link
Contributor Author

xJoeWoo commented Nov 19, 2020

Thanks for the PR.

Regardless of the outcome of this issue, a call to info should not lead to this so please consider reporting that against the Redisson project.

See also https://redis.io/commands/info

It's my pleasure to make Spring Boot better.

Yes, I've made another PR for improving decoding performance in Redisson.
Furthermore, reducing queried result size from source would be a better solution and make benefit to all Redis libraries.

@philwebb
Copy link
Member

I agree with @wilkinsona. I' wondering where we should target this one. Do we consider it a bug?

@snicoll
Copy link
Member

snicoll commented Nov 19, 2020

I was about to ask that, believe it or not. I think this can be considered as such and targeted to 2.2.x.

@philwebb philwebb added type: bug A general bug and removed for: team-attention An issue we'd like other members of the team to review status: waiting-for-triage An issue we've not yet triaged labels Nov 19, 2020
@philwebb philwebb added this to the 2.2.x milestone Nov 19, 2020
@wilkinsona wilkinsona changed the title Reduce redis health indicator info command result size Redis health indicator requests more information than it needs resulting in unnecessarily large responses from Redis Nov 19, 2020
@snicoll snicoll self-assigned this Nov 25, 2020
@snicoll snicoll modified the milestones: 2.2.x, 2.2.12 Nov 25, 2020
@snicoll snicoll closed this in 47efbd9 Nov 25, 2020
@snicoll
Copy link
Member

snicoll commented Nov 25, 2020

@xJoeWoo thank you for making your first contribution to Spring Boot. It has been applied to 2.2.x, 2.3.x and master.

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

Successfully merging this pull request may close these issues.

5 participants