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

Add expire along with hmset #123

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

atn18
Copy link

@atn18 atn18 commented Feb 18, 2020

#101 related ticket

@atn18 atn18 requested a review from rv-kip February 18, 2020 19:56
Copy link

@joshk0 joshk0 left a comment

Choose a reason for hiding this comment

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

Will this actually work?

We get into this scenario if HMSET succeeds but EXPIRE fails. So, you added MULTI so that HMSET and EXPIRE happen atomically. This would work, if the failure of EXPIRE would cause HMSET to rollback.

But according to this page, MULTI's don't cause rollback on errors.

@joshk0
Copy link

joshk0 commented Jul 13, 2020

I created #128 as a competing fix. May the best PR win!

@atn18
Copy link
Author

atn18 commented Jul 14, 2020

I created #128 as a competing fix. May the best PR win!

Seems this repo not supported by contributors, so no PR will win :(

@joshk0
Copy link

joshk0 commented Jul 14, 2020

Ah, you're right. That's so sad!

In the meantime, we could come up with best solution together - what do you think of my approach (and issue I think which happens with yours?)

@rawpixel-vincent
Copy link

some interesting work to test here, too bad the repo is not active, but that happens.
I gave it a try to resurrect it but if nothing happen I will go with #135 as a new project - based on this project dev branch that includes some nice work from @krazyjakee to upgrade the code to ES6
I could use some help with redis, let see what happens and what's not, I might ping you again to includes the best PR into it!

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.

3 participants