Skip to content

Better error handling for updateClientOutputBufferLimit.#9308

Merged
oranagra merged 6 commits intoredis:unstablefrom
enjoy-binbin:client-output-buffer-limit
Aug 29, 2021
Merged

Better error handling for updateClientOutputBufferLimit.#9308
oranagra merged 6 commits intoredis:unstablefrom
enjoy-binbin:client-output-buffer-limit

Conversation

@enjoy-binbin
Copy link
Contributor

@enjoy-binbin enjoy-binbin commented Aug 3, 2021 •
edited
Loading

Refresh #3465
This one follow #9313 and goes deeper (validation of config file parsing)

Move the check/update logic to a new updateClientOutputBufferLimit
function. So that it can be used in CONFIG SET and config file parsing.

Add some changes before and after

Before:

If we set an error value(256fffmb) in redis.conf
client-output-buffer-limit replica 256fffmb 64mb 60

We can start a server and we will get this
127.0.0.1:6379> config get client-output-buffer-limit
1) "client-output-buffer-limit"
2) "normal 0 0 0 slave 0 67108864 60 pubsub 33554432 8388608 60"

If we set an error value(fff60) in redis.conf
client-output-buffer-limit replica 256mb 64mb fff60

We will get this
127.0.0.1:6379> config get client-output-buffer-limit
1) "client-output-buffer-limit"
2) "normal 0 0 0 slave 268435456 67108864 0 pubsub 33554432 8388608 60"

After:

If we set an error value in redis.conf
like: client-output-buffer-limit areplica 256mb 64mb 60f
*** FATAL CONFIG FILE ERROR (Redis 255.255.255) ***
Reading the configuration file, at line 1835
>>> 'client-output-buffer-limit areplica 256mb 64mb 60f'
Invalid client class specified in buffer limit configuration.

like: client-output-buffer-limit replica 256fmb 64mb 60f
*** FATAL CONFIG FILE ERROR (Redis 255.255.255) ***
Reading the configuration file, at line 1835
>>> 'client-output-buffer-limit replica 256fmb 64mb 60f'
Error in hard, soft or soft_seconds setting in buffer limit configuration.

like: client-output-buffer-limit replica 256mb 64mb 60f
*** FATAL CONFIG FILE ERROR (Redis 255.255.255) ***
Reading the configuration file, at line 1835
>>> 'client-output-buffer-limit replica 256mb 64mb 60f'
Error in hard, soft or soft_seconds setting in buffer limit configuration.

@yoav-steinberg yoav-steinberg mentioned this pull request Aug 3, 2021
Closed
yoav-steinberg
yoav-steinberg approved these changes Aug 3, 2021
View reviewed changes
Copy link
Contributor

@yoav-steinberg yoav-steinberg left a comment

Choose a reason for hiding this comment

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

Looks good.

@yoav-steinberg yoav-steinberg added the state:to-be-merged The PR should be merged soon, even if not yet ready, this is used so that it won't be forgotten label Aug 3, 2021
@enjoy-binbin enjoy-binbin mentioned this pull request Aug 22, 2021
Merged
@enjoy-binbin
Better error handling for updateClientOutputBufferLimit.
2300007 
This one follow redis#9313 and goes deeper (validation of config file parsing)

Move the check/update logic to a new updateClientOutputBufferLimit
function. So that it can be used in CONFIG SET and config file parsing.
@enjoy-binbin enjoy-binbin force-pushed the client-output-buffer-limit branch from 418996e to 2300007 Compare August 24, 2021 06:22
enjoy-binbin
enjoy-binbin commented Aug 24, 2021
View reviewed changes
yoav-steinberg
yoav-steinberg reviewed Aug 24, 2021
View reviewed changes
Copy link
Contributor

@yoav-steinberg yoav-steinberg left a comment

Choose a reason for hiding this comment

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

lgtm, minor phrasing issues.

enjoy-binbin and others added 3 commits August 24, 2021 14:47
@enjoy-binbin @yoav-steinberg
Update src/config.c
d5bf3b1 
Co-authored-by: yoav-steinberg <[email protected]>
@enjoy-binbin @yoav-steinberg
Update src/config.c
e7a12e7 
Co-authored-by: yoav-steinberg <[email protected]>
@enjoy-binbin @yoav-steinberg
code review.
78123ef 
Co-authored-by: yoav-steinberg <[email protected]>
yoav-steinberg
yoav-steinberg reviewed Aug 26, 2021
View reviewed changes
Copy link
Contributor

@yoav-steinberg yoav-steinberg left a comment

Choose a reason for hiding this comment

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

I like this.
Added a suggestion that makes sure values are of the correct type.

@enjoy-binbin @yoav-steinberg
Apply yoav's suggestion
a9b2065 
Co-authored-by: yoav-steinberg <[email protected]>
oranagra
oranagra reviewed Aug 26, 2021
View reviewed changes
Copy link
Member

@oranagra oranagra left a comment

Choose a reason for hiding this comment

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

LGTM.

@enjoy-binbin @oranagra
Update src/config.c
0297941 
Co-authored-by: Oran Agra <[email protected]>
@oranagra oranagra merged commit aefbc23 into redis:unstable Aug 29, 2021
@oranagra
Copy link
Member

oranagra commented Aug 29, 2021

@yossigo one could argue this is a breaking change (same as the change in validations i did in #9313), i.e. it would change the outcome of an invalid config from being applied incorrectly to be rejected instead.
do you think we wanna mention this in the release notes?

@enjoy-binbin enjoy-binbin deleted the client-output-buffer-limit branch August 29, 2021 13:16
JackieXie168 pushed a commit to JackieXie168/redis that referenced this pull request Sep 8, 2021
@enjoy-binbin
Better error handling for updateClientOutputBufferLimit. (redis#9308)
5cfbb6e 
This one follow redis#9313 and goes deeper (validation of config file parsing)

Move the check/update logic to a new updateClientOutputBufferLimit
function. So that it can be used in CONFIG SET and config file parsing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@oranagra oranagra oranagra approved these changes

+1 more reviewer

@yoav-steinberg yoav-steinberg yoav-steinberg approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

state:to-be-merged The PR should be merged soon, even if not yet ready, this is used so that it won't be forgotten

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@enjoy-binbin @oranagra @yoav-steinberg