Skip to content

Conversation

ltagliamonte-dd
Copy link
Contributor

@ltagliamonte-dd ltagliamonte-dd commented Jan 28, 2025

New Command HSETEX (HSET+EXPIRE):

HSETEX key TTL f1 v1 f2 v2 f3 v3...

Adding a command that sets an hash key and adds also TTL.
this reduces I/O and improves drastically the performances over pipelining or LUA functions.

This is a common request feature/cmd redis/redis#2905

From my local tests with 8 workers, 600 client connections, over 4 executions on the same dataset.
(branch, TTL, results):
kvrocks/unstable, NO TTL: 392,346,350,346 seconds -> avg -> 358.50 s
kvrocks/unstable, 30d TTL HSET+EXPIRE pipeline: 481,532,483,525 seconds -> avg -> 505.25 s
kvrocks/unstable, 30d TTL using function: 549,499,491,488 seconds -> avg -> 506.75 s
kvrocks/setExpire, 30d TTL with HSETEX: 398,348,342,393 -> avg -> 370.25s

@ltagliamonte-dd ltagliamonte-dd changed the title HSETEX cmd (chore) HSETEX cmd Jan 28, 2025
@ltagliamonte-dd ltagliamonte-dd changed the title (chore) HSETEX cmd (feat HSETEX) Hset+Expire cmd Jan 28, 2025
@ltagliamonte-dd ltagliamonte-dd changed the title (feat HSETEX) Hset+Expire cmd feat(HSETEX) Hset+Expire cmd Jan 28, 2025
@PragmaTwice PragmaTwice changed the title feat(HSETEX) Hset+Expire cmd feat(hash): add the support of HSETEX command (HSET + EXPIRE) Jan 28, 2025
Copy link
Member

@PragmaTwice PragmaTwice left a comment

Choose a reason for hiding this comment

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

Looks good to me. cc @git-hulk

@PragmaTwice PragmaTwice requested a review from git-hulk January 28, 2025 14:20
git-hulk
git-hulk previously approved these changes Jan 28, 2025
Copy link
Member

@git-hulk git-hulk left a comment

Choose a reason for hiding this comment

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

LGTM, it would be great if we can add a few test cases for this command in the further PR.

@ltagliamonte-dd
Copy link
Contributor Author

LGTM, it would be great if we can add a few test cases for this command in the further PR.

writing test cases, will push here as soon i'm done

@PragmaTwice
Copy link
Member

Yeah it's better to include test cases in this PR.

@ltagliamonte-dd
Copy link
Contributor Author

@PragmaTwice @git-hulk pushed unit tests.
thanks for the amazing support!

@PragmaTwice PragmaTwice requested review from torwig and mapleFU January 29, 2025 09:41
git-hulk
git-hulk previously approved these changes Jan 29, 2025
@PragmaTwice
Copy link
Member

PragmaTwice commented Jan 29, 2025

I just found one issue about the command name: HSETEX.

HSETEX can be HSET + EXPIRE, but it also can be HSET + HEXPIRE (with the hash field expiration).

Redis doesn't have this command (and, potentially will add this command in the future).

Dragonfly has this command (https://www.dragonflydb.io/docs/command-reference/hashes/hsetex), which means HSET + HEXPIRE (e.g. HSETEX key ttl k1 v1 will add the ttl to only k1 field, instead of the whole key).

So if we add this, there's a potentially semantic inconsistence: if Redis add it in the future and the Redis one means HSET + HEXPIRE, we'll have a trouble, i.e. we will be incompatible to Redis, otherwise incompatible to previous version of Kvrocks. (and also confuse our users.)

cc @git-hulk

Maybe we can rename it to HSETEXPIRE.

@git-hulk
Copy link
Member

Dragonfly has this command (https://www.dragonflydb.io/docs/command-reference/hashes/hsetex), which means HSET + HEXPIRE (e.g. HSETEX key ttl k1 v1 will add the ttl to only k1 field, instead of the whole key).

@PragmaTwice @ltagliamonte-dd

Good finding. I didn't notice Dragonfly had this command. HSETEXPIRE sounds good to me.

@PragmaTwice
Copy link
Member

PragmaTwice commented Jan 29, 2025

Good finding. I didn't notice Dragonfly had this command. HSETEXPIRE sounds good to me.

Great! Let's change it to HSETEXPIRE. cc @ltagliamonte-dd

@PragmaTwice PragmaTwice changed the title feat(hash): add the support of HSETEX command (HSET + EXPIRE) feat(hash): add the support of HSETEXPIRE command (HSET + EXPIRE) Jan 29, 2025
@ltagliamonte-dd
Copy link
Contributor Author

@PragmaTwice @git-hulk Rename addressed via 6f28ae0

Copy link

@PragmaTwice PragmaTwice merged commit f7c5688 into apache:unstable Jan 30, 2025
35 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.

3 participants