-
Notifications
You must be signed in to change notification settings - Fork 63
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
RadisChaos: Add Cache Expiration #175
Conversation
Signed-off-by: FingerLeader <wanxfinger@gmail.com>
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
I find |
It works now |
Signed-off-by: FingerLeader <wanxfinger@gmail.com>
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.
Please fix the ci, rest LGTM
Signed-off-by: FingerLeader <wanxfinger@gmail.com>
Signed-off-by: FingerLeader <wanxfinger@gmail.com>
Signed-off-by: FingerLeader <wanxfinger@gmail.com>
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.
LGTM
cmd/attack/redis.go
Outdated
cmd.Flags().StringVarP(&options.Addr, "addr", "a", "", "The address of redis server") | ||
cmd.Flags().StringVarP(&options.Password, "password", "p", "", "The password of server") | ||
cmd.Flags().StringVarP(&options.Key, "key", "k", "", "The key to be set a expiration, default expire all keys") | ||
cmd.Flags().StringVarP(&options.Expiration, "expiration", "", "0", "The expiration of the key") |
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.
Does this flag represent expiration time? Can we add an example of this flag?
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.
Yes, and I'll enrich the description of expiration
.
pkg/core/redis.go
Outdated
@@ -60,6 +64,10 @@ func (r *RedisCommand) Validate() error { | |||
if r.CacheSize != "0" && r.Percent != "" { | |||
return errors.New("only one of cachesize and percent can be set") | |||
} | |||
case RedisCacheExpirationAction: | |||
if r.Option != "" && r.Option != "XX" && r.Option != "NX" && r.Option != "GT" && r.Option != "LT" { |
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.
This check is ugly. We can add use a map or an array to improve it.
pkg/server/chaosd/redis.go
Outdated
} | ||
|
||
func ExpireFunc(cli *redis.Client, key string, expiration time.Duration, option string) *redis.BoolCmd { | ||
if option == "NX" { |
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.
I prefer to use switch
instead of if else
@FingerLeader PTAL |
Signed-off-by: FingerLeader <wanxfinger@gmail.com>
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.
LGTM
/merge |
This pull request has been accepted and is ready to merge. Commit hash: e5a349c
|
Signed-off-by: FingerLeader wanxfinger@gmail.com