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.

Sign up for GitHub

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

Jump to bottom

#570: Adds support for SETEX command #590

Merged
merged 6 commits into from
Sep 23, 2024
Merged

#570: Adds support for SETEX command #590

merged 6 commits into from
Sep 23, 2024

Conversation

AshikBN
Copy link
Contributor

@AshikBN AshikBN commented Sep 15, 2024

Added support for SETEX command #570

@lucifercr07
Copy link
Contributor

@AshikBN please rebase to resolve conflicts.

@AshikBN
Copy link
Contributor Author

AshikBN commented Sep 18, 2024

rebase is done.pls check @lucifercr07

JyotinderSingh
JyotinderSingh requested changes Sep 19, 2024
View reviewed changes
Copy link
Collaborator

@JyotinderSingh JyotinderSingh left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Let's implement setex based on set the way @kushal0511-not has suggested.
Please rebase on latest master and resolve conflicts.

internal/eval/eval_test.go Outdated Show resolved Hide resolved
internal/eval/eval_test.go Outdated Show resolved Hide resolved
@AshikBN
570:SETEX command Feature
af5d538 
resolved the conflicts and updated evalsetex to call evalset func
@AshikBN
Copy link
Contributor Author

AshikBN commented Sep 21, 2024

made the relevant changes, pls review @kushal0511-not

@kushal0511-not
Copy link
Contributor

Thanks for changes @AshikBN . Could you please address @JyotinderSingh 's comments also?
#590 (comment) and #590 (comment)

@AshikBN
Copy link
Contributor Author

AshikBN commented Sep 21, 2024

done @kushal0511-not

@kushal0511-not
Copy link
Contributor

@AshikBN Thanks for changes. LGTM.
Just add comment to function.

@AshikBN
Copy link
Contributor Author

AshikBN commented Sep 21, 2024

added comments @kushal0511-not

@JyotinderSingh
Copy link
Collaborator

Thanks for adding support for the SETEX command, @AshikBN! Thank you for the reviews @kushal0511-not.

The changes had gone out of date and weren't compatible with the new code anymore, since the SET command has been migrated to the new multi-shard setup. I have patched my changes into this PR to migrate SETEX as well.

Merging this now.

@JyotinderSingh JyotinderSingh changed the title #570:SETEX command Feature #570: Adds support for SETEX command Sep 23, 2024
@JyotinderSingh JyotinderSingh merged commit 9ab018c into DiceDB:master Sep 23, 2024
2 checks passed
shardul08 pushed a commit to shardul08/dice that referenced this pull request Sep 23, 2024
@AshikBN @JyotinderSingh @shardul08
DiceDB#570: Adds support for SETEX command (DiceDB#590)
555dd63 
Co-authored-by: Jyotinder Singh <jyotindrsingh@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kushal0511-not kushal0511-not kushal0511-not left review comments

@JyotinderSingh JyotinderSingh JyotinderSingh requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@AshikBN @lucifercr07 @kushal0511-not @JyotinderSingh