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

support for CLIENT KILL MAXAGE #2727

Merged
merged 8 commits into from
Jul 9, 2024
Merged

Conversation

atakavci
Copy link
Contributor

Closes/Fixes #2726

added two overloads and use a filter object to avoid a breaking change.

@NickCraver
Copy link
Collaborator

Reviewed on the call today - in general, this pattern won't work due to the server-side API supporting n filters, we'd need to allow n of them ordered on the command. We'd either need to take a params here or more likely do a builder pattern for n filters. In a builder, returning itself, we could do things like making a method per filter that's very specific and only has the args needed for the 7 doced paths:

  • CLIENT KILL ADDR ip:port
  • CLIENT KILL LADDR ip:port
  • CLIENT KILL ID client-id
  • CLIENT KILL TYPE type
  • CLIENT KILL USER username
  • CLIENT KILL SKIPME yes/no
  • CLIENT KILL MAXAGE maxage

This wouldn't be very dissimilar to the constructors you have now in terms of one-per, but we can make it intuitive and pit-of-success here for users to do the right thing. And we can make it write the message directly on the backend since it's in the base library. Picture for example:

IServer.ClientKillAsync(new ClientKillFilter().WithAddr(...).WithSkipMe(true));`

Thoughts?

@atakavci
Copy link
Contributor Author

hey @NickCraver , sounds nice and clear ! please take a look with new commit.

Copy link
Collaborator

@NickCraver NickCraver left a comment

Choose a reason for hiding this comment

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

Took a look here local and pushed up some fixes, but it looks like we don't implement the LADDR or USER filters in the actual send, so we need to add those and preferably add tests around the filter calling each command and making sure these are set.

@atakavci
Copy link
Contributor Author

atakavci commented Jul 3, 2024

@NickCraver , yeah i missed that two. Adding them..
Now i am looking for convenient way to check the arguments of the Message in unit test. Could you spot a sample ?

@atakavci atakavci requested a review from NickCraver July 3, 2024 15:40
Copy link
Collaborator

@NickCraver NickCraver left a comment

Choose a reason for hiding this comment

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

Latest is looking good - thank you!

@NickCraver NickCraver merged commit a64ca14 into StackExchange:main Jul 9, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support MAXAGE option for CLIENT KILL
2 participants