Skip to content

Conversation

@chendq8
Copy link
Contributor

@chendq8 chendq8 commented Aug 19, 2022

Check the validity of the value before performing the create operation, prevents new data from being generated even if the request fails to execute.

Copy link
Contributor

@enjoy-binbin enjoy-binbin left a comment

Choose a reason for hiding this comment

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

can you add some top comment to describe the problem?
didn't bother to read through the code

@oranagra
Copy link
Member

oranagra commented Aug 21, 2022

indeed. a detailed comment would help. @chendq8 please update.
looking at the test, i realize you found a bug in which a command can fail (return an error), but it could happen after already performing a medication to the database.

however, i don't think i like the way to fixed it (changing hashTypeLookupWriteOrCreate to it's components).
how about just copying the isnan and isinf to the top?
i.e. if the input isn't Inf or NaN, then adding 0 won't change it to be one of them.
or in other words, the first check will check the input, and the second one will only be relevant if the key already exists.

it does mean that we'll affect the error, e.g. cases that in the past would result with WRONGTYPE, will now be rejected with the NaN/Inf error, but i don't think that's an issue.

@chendq8
Copy link
Contributor Author

chendq8 commented Aug 22, 2022

indeed. a detailed comment would help. @chendq8 please update. looking at the test, i realize you found a bug in which a command can fail (return an error), but it could happen after already performing a medication to the database.

however, i don't think i like the way to fixed it (changing hashTypeLookupWriteOrCreate to it's components). how about just copying the isnan and isinf to the top? i.e. if the input isn't Inf or NaN, then adding 0 won't change it to be one of them. or in other words, the first check will check the input, and the second one will only be relevant if the key already exists.

it does mean that we'll affect the error, e.g. cases that in the past would result with WRONGTYPE, will now be rejected with the NaN/Inf error, but i don't think that's an issue.

The implementation mode has been modified.

Copy link
Contributor

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

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

LGTM

@oranagra oranagra changed the title fix hincrbyfloat fix hincrbyfloat not to create a key if the new value is invalid Aug 28, 2022
@oranagra oranagra merged commit bc7fe41 into redis:unstable Aug 28, 2022
@oranagra oranagra added the release-notes indication that this issue needs to be mentioned in the release notes label Aug 28, 2022
oranagra added a commit that referenced this pull request Apr 17, 2023
)

Check the validity of the value before performing the create operation,
prevents new data from being generated even if the request fails to execute.

Co-authored-by: Oran Agra <[email protected]>
Co-authored-by: chendianqiang <[email protected]>
Co-authored-by: Binbin <[email protected]>
(cherry picked from commit bc7fe41)
(cherry picked from commit 606a385)
oranagra added a commit that referenced this pull request Apr 17, 2023
)

Check the validity of the value before performing the create operation,
prevents new data from being generated even if the request fails to execute.

Co-authored-by: Oran Agra <[email protected]>
Co-authored-by: chendianqiang <[email protected]>
Co-authored-by: Binbin <[email protected]>
(cherry picked from commit bc7fe41)
oranagra added a commit that referenced this pull request Apr 17, 2023
)

Check the validity of the value before performing the create operation,
prevents new data from being generated even if the request fails to execute.

Co-authored-by: Oran Agra <[email protected]>
Co-authored-by: chendianqiang <[email protected]>
Co-authored-by: Binbin <[email protected]>
(cherry picked from commit bc7fe41)
(cherry picked from commit 606a385)
(cherry picked from commit 7df23a5)
madolson pushed a commit to madolson/redis that referenced this pull request Apr 19, 2023
…is#11149)

Check the validity of the value before performing the create operation,
prevents new data from being generated even if the request fails to execute.

Co-authored-by: Oran Agra <[email protected]>
Co-authored-by: chendianqiang <[email protected]>
Co-authored-by: Binbin <[email protected]>
enjoy-binbin added a commit to enjoy-binbin/redis that referenced this pull request Jul 31, 2023
…is#11149)

Check the validity of the value before performing the create operation,
prevents new data from being generated even if the request fails to execute.

Co-authored-by: Oran Agra <[email protected]>
Co-authored-by: chendianqiang <[email protected]>
Co-authored-by: Binbin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-notes indication that this issue needs to be mentioned in the release notes

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants