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

Add io-threads-do-reads config to deprecated config table to have no effect. #1138

Merged
merged 4 commits into from
Oct 10, 2024

Conversation

Shivshankar-Reddy
Copy link
Contributor

this fixes: #1116

Issue details from #1116 by @zuiderkwast

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)

…effect on io-threads

Signed-off-by: Shivshankar-Reddy <[email protected]>
Copy link

codecov bot commented Oct 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (unstable@e617bf2). Learn more about missing BASE report.
Report is 6 commits behind head on unstable.

Additional details and impacted files
@@             Coverage Diff             @@
##             unstable    #1138   +/-   ##
===========================================
  Coverage            ?   70.59%           
===========================================
  Files               ?      114           
  Lines               ?    61716           
  Branches            ?        0           
===========================================
  Hits                ?    43567           
  Misses              ?    18149           
  Partials            ?        0           
Files with missing lines Coverage Δ
src/config.c 78.69% <ø> (ø)
src/io_threads.c 7.90% <ø> (ø)
src/server.h 100.00% <ø> (ø)

Copy link
Contributor

@melroy89 melroy89 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again please update documentation and example config file explaining this option is deprecated and doesn't do anything anymore.

@Shivshankar-Reddy
Copy link
Contributor Author

Shivshankar-Reddy commented Oct 9, 2024

Added a note in the valkey.conf file as it is deprecated and avoid using it in future. Thanks for remainding it.

@melroy89
Copy link
Contributor

melroy89 commented Oct 9, 2024

Small typo in title: depricated -> deprecated

@madolson madolson changed the title Add io-threads-do-reads config to depricated config table to have no effect. Add io-threads-do-reads config to deprecated config table to have no effect. Oct 9, 2024
valkey.conf Outdated Show resolved Hide resolved
Signed-off-by: Shivshankar-Reddy <[email protected]>
@zuiderkwast zuiderkwast merged commit 079f18a into valkey-io:unstable Oct 10, 2024
47 checks passed
@madolson madolson added the release-notes This issue should get a line item in the release notes label Oct 10, 2024
eifrah-aws pushed a commit to eifrah-aws/valkey that referenced this pull request Oct 20, 2024
…effect. (valkey-io#1138)

this fixes: valkey-io#1116

_Issue details from valkey-io#1116 by @zuiderkwast_ 

> This config is undocumented since valkey-io#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 [valkey-io#1019 (reply in
thread)](https://github.com/orgs/valkey-io/discussions/1019#discussioncomment-10824778)_

---------

Signed-off-by: Shivshankar-Reddy <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes This issue should get a line item in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make io-threads-do-reads harmless
4 participants