-
Notifications
You must be signed in to change notification settings - Fork 748
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
Log the real reason for why posix_fadvise failed #430
Conversation
`reclaimFilePageCache` did not set `errno` but `rdbSaveInternal` which is logging the error assumed it did. This makes sure `errno` is set. Fixes valkey-io#429 Signed-off-by: Ted Lyngmo <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #430 +/- ##
=========================================
Coverage 68.44% 68.45%
=========================================
Files 109 109
Lines 61671 61673 +2
=========================================
+ Hits 42209 42216 +7
+ Misses 19462 19457 -5
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me, given https://man7.org/linux/man-pages/man2/posix_fadvise.2.html.
Did you encounter this issue while running in production or just reviewing code. This will influence whether or not we backport the fix.
I noticed it in our test environment when I tried tracking down reasons for misc. "Resource temporarily unavailable" issues - so I don't really know the real reason for why I also added a PR in Redis: redis/redis#13246 |
OK, sounds like enough reason to backport. |
@madolson It looks like this didn't make it into 7.2.6, but I do see it in 8.0.0-rc1. Was that a mistake or how are backports released? |
@TedLyngmo That was a mistake, we didn't have our backporting strategy sorted correctly. It will be in 7.2.7 which is getting released tomorrow. |
`reclaimFilePageCache` did not set `errno` but `rdbSaveInternal` which is logging the error assumed it did. This makes sure `errno` is set. Signed-off-by: Ted Lyngmo <[email protected]>
reclaimFilePageCache
did not seterrno
butrdbSaveInternal
which is logging the error assumed it did. This makes sureerrno
is set.Fixes #429