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

Enhancement: Append to support other datatypes #1193

Closed
4 tasks
apoorvyadav1111 opened this issue Oct 24, 2024 · 10 comments · Fixed by #1237
Closed
4 tasks

Enhancement: Append to support other datatypes #1193

apoorvyadav1111 opened this issue Oct 24, 2024 · 10 comments · Fixed by #1237

Comments

@apoorvyadav1111
Copy link
Contributor

apoorvyadav1111 commented Oct 24, 2024

Hi team,

Command Append has recently been migrated and missed support for all available datatypes. We need to make it consistent with Redis. For more context, please go through #1095 comments and unresolved review comments.

Requirements:

  • All datatypes are supported and error handling must be consistent.
  • updating unit tests to cover new enhancement
  • updating all integration test to test new enhancement
  • documentation updates with new enhancement

Thanks

@shashi-sah2003
Copy link
Contributor

@apoorvyadav1111 if noone is assigned on this then I would like to take this
thanks

@apoorvyadav1111
Copy link
Contributor Author

Thanks for picking this up @shashi-sah2003.

@shashi-sah2003
Copy link
Contributor

@apoorvyadav1111 the command APPEND works only with string datatype and it will throw error when used with other datatype
you want me to check for all the other datatypes if it's throwing error or not?

@apoorvyadav1111
Copy link
Contributor Author

Hi @shashi-sah2003 , yes please, we need to handle all datatypes which should be stringified. We need to be consistent with redis, hence, please check the behavior on redis.

@shashi-sah2003
Copy link
Contributor

image
@apoorvyadav1111 when I'm trying to append in sorted set then it should give relevant error but the server is being closed immediately and the below error is appeared on terminal, rest all other datatypes is giving relevant error/output
image

@apoorvyadav1111
Copy link
Contributor Author

Hi @shashi-sah2003 , Can you please create a draft pr with the code throwing this error? Thanks

@shashi-sah2003
Copy link
Contributor

shashi-sah2003 commented Nov 2, 2024

@apoorvyadav1111 I have created draft PR for the above issue with APPEND command pls review it #1233
also how should I go about fixing this issue or from where it is arising from?
thanks

@c-harish
Copy link
Contributor

c-harish commented Nov 2, 2024

@apoorvyadav1111 I have created draft PR for the above issue with APPEND command pls review it #1233 also how should I go about fixing this issue or from where it is arising from? thanks

Hi @shashi-sah2003. Are you working on this panic exit? If not, I can raise a fix for it.

@apoorvyadav1111
Copy link
Contributor Author

Hi @shashi-sah2003 , I ll take a look today afternoon and we can move ahead from there.

Thanks,
Apoorv

@shashi-sah2003
Copy link
Contributor

@apoorvyadav1111 I have created draft PR for the above issue with APPEND command pls review it #1233 also how should I go about fixing this issue or from where it is arising from? thanks

Hi @shashi-sah2003. Are you working on this panic exit? If not, I can raise a fix for it.

Hey @c-harish I have been assigned this issue and working on it. Instead it would be helpful if you can give your insight on solving this issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants