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

Make io-threads-do-reads harmless #1116

Open
zuiderkwast opened this issue Oct 2, 2024 · 4 comments
Open

Make io-threads-do-reads harmless #1116

zuiderkwast opened this issue Oct 2, 2024 · 4 comments
Assignees
Labels
good first issue Good for newcomers help wanted External contributions would be appreciated

Comments

@zuiderkwast
Copy link
Contributor

This config is undocumented since #758. The default was changed to "yes" and it is quite useless to set it to "no". Yet, it can happen that some user has an old config file where it is explicitly set to "no". The result will be bad performace, since I/O threads will not do all the I/O.


It's indeed confusing.

  1. Either remove the whole option from the code. And thus no need for documentation. OR:
  2. Introduce the option back in the configuration, just as a comment is fine. And showing the default value "yes": # io-threads-do-reads yes with additional text.

Originally posted by @melroy89 in #1019 (reply in thread)

@zuiderkwast
Copy link
Contributor Author

zuiderkwast commented Oct 2, 2024

If we remove the option completely from the code, valkey refuses to start if the option is present in config. I think it's better to keep it in the code but make it so it has no effect at all.

@melroy89
Copy link
Contributor

melroy89 commented Oct 2, 2024

Since this option is known in Redis still, but you don't want to break backwards compatibility (which I fully understand). After making this option "harmless", I would still only mention the option.. Only mentioning that this option doesn't do anything would be sufficient to remove any confusion by the users.

@zuiderkwast zuiderkwast added the help wanted External contributions would be appreciated label Oct 2, 2024
@madolson madolson added the good first issue Good for newcomers label Oct 4, 2024
@madolson
Copy link
Member

madolson commented Oct 4, 2024

To make this easier for newcomers to pickup:

  1. We need to remove the reference here
  2. We need to add the config to the list of deprecated configs

@Shivshankar-Reddy
Copy link
Contributor

I will start working on this, Please let me know if someone already started working. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted External contributions would be appreciated
Projects
None yet
Development

No branches or pull requests

4 participants