Skip to content

Allow most CONFIG SET during loading, block some commands in async-loading#9878

Merged
oranagra merged 8 commits intoredis:unstablefrom
oranagra:config_set_loading
Dec 22, 2021
Merged

Allow most CONFIG SET during loading, block some commands in async-loading#9878
oranagra merged 8 commits intoredis:unstablefrom
oranagra:config_set_loading

Conversation

@oranagra
Copy link
Member

@oranagra oranagra commented Dec 1, 2021 •
edited
Loading

background

Till now CONFIG SET was blocked during loading.
(In the not so distant past, GET was disallowed too)

We recently (not released yet) added an async-loading mode, see #9323, and during that time it'll serve CONFIG SET and any other command.
And now we realized (#9770) that some configs, and commands are dangerous during async-loading.

changes

  • Allow most CONFIG SET during loading (both on async-loading and normal loading)
  • Allow CONFIG REWRITE and CONFIG RESETSTAT during loading
  • Block a few config during loading (appendonly, repl-diskless-load, and dir)
  • Block a few commands during loading (list below)

the blocked commands:

  • SAVE - obviously we don't wanna start a foregreound save during loading 8-)
  • BGSAVE - we don't mind to schedule one, but we don't wanna fork now
  • BGREWRITEAOF - we don't mind to schedule one, but we don't wanna fork now
  • MODULE - we obviously don't wanna unload a module during replication / rdb loading (MODULE HELP and MODULE LIST are not blocked)
  • SYNC / PSYNC - we're in the middle of RDB loading from master, must not allow sync requests now.
  • REPLICAOF / SLAVEOF - we're in the middle of replicating, maybe it makes sense to let the user abort it, but he couldn't do that so far, i don't wanna take any risk of bugs due to odd state.
  • CLUSTER - only allow [HELP, SLOTS, NODES, INFO, MYID, LINKS, KEYSLOT, COUNTKEYSINSLOT, GETKEYSINSLOT, RESET, REPLICAS, COUNT_FAILURE_REPORTS], for others, preserve the status quo

other fixes

todo

  • convert the hard coded list of commands to block to command flags
  • remove SENTINEL (it's not allowed in loading anyway)
  • make sure to add the flag to both SLAVEOF and REPLICAOF
  • block most CLUSTER sub-commands, only allow the ones listed above.

@oranagra oranagra requested review from soloestoy and yoav-steinberg and removed request for yoav-steinberg December 1, 2021 17:53
@oranagra oranagra mentioned this pull request Dec 1, 2021
Closed
@oranagra oranagra added 7.0-must-have release-notes indication that this issue needs to be mentioned in the release notes state:major-decision Requires core team consensus labels Dec 1, 2021
yoav-steinberg
yoav-steinberg reviewed Dec 2, 2021
View reviewed changes
@oranagra
Merge remote-tracking branch 'origin/unstable' into config_set_loading
3f52a8c
@oranagra
Change async loading blocking to use command flags
2bfea4d 
unblock MODULE HELP and MODULE LIST
unblock CLUSTER sub-commands: HELP, SLOTS, NODES, INFO, MYID, LINKS, KEYSLOT,
  COUNTKEYSINSLOT, GETKEYSINSLOT
Allow CONFIG SET, CONFIG RESETSTAT, CONFIG REWRITE
@oranagra oranagra added the approval-needed Waiting for core team approval to be merged label Dec 16, 2021
@oranagra
Copy link
Member Author

oranagra commented Dec 16, 2021

@redis/core-team please approve, see top comment for details.

@oranagra oranagra mentioned this pull request Dec 16, 2021
Merged
madolson
madolson approved these changes Dec 21, 2021
View reviewed changes
Copy link
Contributor

@madolson madolson left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Approving the 'high level design' but still have my one comment around cluster.

Edit: my comment was addressed, so approving details as well.

@oranagra oranagra merged commit 41e6e05 into redis:unstable Dec 22, 2021
@oranagra oranagra deleted the config_set_loading branch December 22, 2021 12:11
@sundb
Copy link
Collaborator

sundb commented Dec 22, 2021

Fully CI failed: https://github.com/sundb/redis/runs/4606471167?check_suite_focus=true

@oranagra oranagra mentioned this pull request Dec 22, 2021
Merged
oranagra added a commit that referenced this pull request Dec 22, 2021
@oranagra
resolve replication test timing sensitivity (#9982)
e33e029 
issue started failing after #9878 was merged (made an exiting test more sensitive)
@oranagra oranagra mentioned this pull request Dec 22, 2021
Merged
oranagra added a commit that referenced this pull request Dec 22, 2021
@oranagra
resolve replication test timing sensitivity - 2nd attempt (#9988)
b756739 
issue started failing after #9878 was merged (made an exiting test more sensitive)
looks like #9982 didn't help, tested this one and it seems to work better.

this commit does two things:
1. reduce the extra delay i added earlier and instead add more keys, the effect no duration
   of replication is the same, but the intervals in which the server is responsive to the tcl client is higher.
2. improve the test infra to print context when assert_error fails.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yossigo yossigo yossigo approved these changes

@itamarhaber itamarhaber itamarhaber approved these changes

@soloestoy soloestoy Awaiting requested review from soloestoy

+3 more reviewers

@yoav-steinberg yoav-steinberg yoav-steinberg left review comments

@hwware hwware hwware left review comments

@madolson madolson madolson approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

approval-needed Waiting for core team approval to be merged release-notes indication that this issue needs to be mentioned in the release notes state:major-decision Requires core team consensus

Projects

Redis 7.0
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@oranagra @sundb @yoav-steinberg @yossigo @itamarhaber @madolson @hwware