feat(hash): add support of hash expiration commands by ltagliamonte-dd · Pull Request #2717 · apache/kvrocks

ltagliamonte-dd · 2025-01-09T00:27:28Z

based on #2402

fixed CommandHStrlen return
removed keyExpirationCheck in GetMetadata as per @PragmaTwice request
verified all cmds

@jjz921024 I verified all hash cmds and they seem to work without the additional check in GetMetadata do you happen to remember if there was a specific use case you added it there?

jjz921024 · 2025-01-09T12:20:24Z

@ltagliamonte-dd Hi. Did you set this config hash-field-expiration to yes or run the unit tests in this branch? If the field has an expiration, we have to check to the field expiration before each read operation.

ltagliamonte-dd · 2025-01-09T20:15:47Z

@ltagliamonte-dd Hi. Did you set this config hash-field-expiration to yes or run the unit tests in this branch? If the field has an expiration, we have to check to the field expiration before each read operation.

Hello @jjz921024 thanks for getting back to me, yes I did test all hash commands having hash-field-expiration yes.
I plan to give another looks at the code today, i'd love to get in touch with you to better understand the check because from what i could briefly see the key expiration check is also done in each cmds implementations.
I coudn't find your contact neither on the kvrocks slack nor zulipchat.
i did run the unit test with no problem the go integration test only a test case is failing (i plan to look at it today):

--- FAIL: TestHashFieldExpiration (7.66s)
    --- FAIL: TestHashFieldExpiration/HFE_check_hash_metadata_after_all_of_fields_expired (1.00s)
        hash_test.go:1018: 
            	Error Trace:	/Users/luigi.tagliamonte/Projects/doordash/kvrocks/tests/gocase/unit/type/hash/hash_test.go:1018
            	Error:      	Not equal: 
            	            	expected: -2ns
            	            	actual  : -1ns
            	Test:       	TestHashFieldExpiration/HFE_check_hash_metadata_after_all_of_fields_expired
FAIL
exit status 1
FAIL	github.com/apache/kvrocks/tests/gocase/unit/type/hash	199.945s

ltagliamonte · 2025-01-10T03:07:10Z

@jjz921024 the CI seems to pass removing the check in GetMetadata and fixing GetTime.
Would love to get it touch with you to see if I'm missing something here.

git-hulk · 2025-01-11T04:54:33Z

@jjz921024 @ltagliamonte-dd Thanks for your great efforts. I took my first pass in a new PR and generally look in good shape from my perspective. Two nit points can be improved:

Change ExistValidField to GetValidFieldCount, so that Hash::Size can also resue this function.
Many places decode the hash metadata and check if it has any valid fields. We can avoid duplicate code by adding a dedicated function to this check.

    if (metadata.Type() == kRedisHash) {
      HashMetadata hash_metadata(false);
      s = hash_metadata.Decode(rocksdb::Slice(pin_values[i].data(), pin_values[i].size()));
      if (!s.ok()) continue;
      redis::Hash hash_db(storage_, namespace_);
      if (hash_db.ExistValidField(ctx, slice_keys[i], hash_metadata)) {

jjz921024 · 2025-01-12T12:32:25Z

@ltagliamonte-dd Hi, I create a PR #1 in your kvrocks fork repo. There are some polish about this feature for your reference.

The purpose of this this commit aac63cc is to avoid unnecessary field expiration checks when executing the hset/hmset command, which comes from #2402 (comment) @PragmaTwice suggestion.

The purpose of this this commit 7306231 is make ExistValidField method more general so that Hash::Size can also resue it. which comes from @git-hulk suggestion.

ltagliamonte-dd · 2025-01-13T17:40:40Z

Hello @jjz921024 thank you for the help on getting this through the finish line.
As I looked at the PR ltagliamonte-dd#1 I can see that GetMetadata still contains the expiration check on all fields keys, that @PragmaTwice was pointing out as possible perf problem.

Discussing this check with @PragmaTwice and @git-hulk on zulipchat we decided to move the check up in the call stack and perform it only when required.

As the CI on this PR is showing (and my verification) it looks like there is no need for expiration check in GetMetadata, I would love to hear from you if i'm missing something and why the check is required. Please join us on zulipchat so we can discuss this in a more interactive way.

by removing the check in GetMetadata commit aac63cc is not needed

thank you for commit 7306231 i will be importing it.

ltagliamonte-dd · 2025-01-13T23:21:43Z

@git-hulk I gave a look at your suggestion to remove duplicated code, it looks like duplicated but if you look closely they are all slightly different checks... we may be able to refactor with something like this:

rocksdb::Status Hash::HandleRedisHash(engine::Context &ctx, const Metadata &metadata, const std::string &value,
                                  const std::string &key, std::function<void(int)> on_valid_field_count);

so we pass an handleFunction into the refactored function
Happy to hear if you have better/different ideas about it.

PragmaTwice · 2025-01-22T03:00:10Z

I'll take a look at this weekend if no other things happen : )

cc @mapleFU PTAL.

Also now we need to solve git conflicts before reviewing and merging. cc @ltagliamonte-dd

ltagliamonte-dd · 2025-01-22T10:10:03Z

@PragmaTwice can you please re-trigger the CI it looks like a flaky test

kvrocks.conf

PragmaTwice · 2025-01-25T08:18:10Z

src/types/redis_hash.cc

+rocksdb::Status Hash::encodeExpireToValue(std::string *value, uint64_t expire) {
+  std::string buf;
+  PutFixed64(&buf, expire);
+  buf.append(*value);
+  value->assign(buf.data(), buf.size());
+  return rocksdb::Status::OK();
+}


Suggested change

rocksdb::Status Hash::encodeExpireToValue(std::string *value, uint64_t expire) {

std::string buf;

PutFixed64(&buf, expire);

buf.append(*value);

value->assign(buf.data(), buf.size());

return rocksdb::Status::OK();

}

rocksdb::Status Hash::encodeExpireAndValue(std::string_view value, uint64_t expire, std::string *out) {

PutFixed64(out, expire);

out->append(value);

return rocksdb::Status::OK();

}

Also for all places that uses this function.

@PragmaTwice I have trouble understanding the suggestion.. would be this correct:

rocksdb::Status Hash::encodeExpireAndValue(std::string value, uint64_t expire, std::string *enc_value) { PutFixed64(enc_value, expire); enc_value->append(value); return rocksdb::Status::OK(); }

I think string_view should be better here.

@PragmaTwice please let me know how 3a726e1 looks, happy to address further if needed.

@PragmaTwice string_view comment addressed via b10d710

Ahh I revisited it and I think it can be std::string Hash::encodeExpireAndValue(std::string_view value, uint64_t expire).

The returned status here seems useless.

std::string Hash::encodeExpireAndValue(std::string_view value, uint64_t expire) { std::string out; PutFixed64(&out, expire); out.append(value); return out; }

thanks addressed via 9bbc8d9

PragmaTwice

I'm thinking that, size in hash metadata will become nearly a useless field while expiration is enabled, so it's a little tricky since we can easily misuse it. cc @git-hulk @mapleFU

git-hulk · 2025-01-25T09:25:59Z

I'm thinking that, size in hash metadata will become nearly a useless field while expiration is enabled, so it's a little tricky since we can easily misuse it. cc @git-hulk @mapleFU

Yes, we need to take care of this after introducing the hash expiration feature. But we can try to improve this before formally releasing it.

Co-authored-by: Twice <twice@apache.org>

sonarqubecloud · 2025-01-26T14:08:26Z

Quality Gate passed

Issues
40 New issues
0 Accepted issues

Measures
0 Security Hotspots
69.9% Coverage on New Code
12.6% Duplication on New Code

See analysis details on SonarQube Cloud

ltagliamonte-dd · 2025-02-10T22:46:41Z

Hello Folks any updates on review/merge this PR?
Cc @PragmaTwice @git-hulk ?

ltagliamonte-dd · 2025-03-04T19:46:37Z

Folks do we have any updates on this?
@PragmaTwice @git-hulk

sonarqubecloud · 2025-03-04T22:45:52Z

Quality Gate passed

Issues
41 New issues
0 Accepted issues

Measures
0 Security Hotspots
69.1% Coverage on New Code
13.9% Duplication on New Code

See analysis details on SonarQube Cloud

PragmaTwice · 2025-03-05T02:51:45Z

Hi @ltagliamonte-dd , sorry for late response.

This PR includes massive changes to existing, critical code (the encoding logic of HASH data structure). So it requires lots of time to review to ensure that, not only users that need this feature, but also users that don't care this feature, can have a good and consistent experience while using Kvrocks. Also we need to make sure it doesn't affect old users, and the performance doesn't have a big drop.

I currently don't have much time to complete a full review due to some personal affairs. Not sure if @git-hulk and @mapleFU have time to review. But if I get some time I'll move on for this PR : )

git-hulk · 2025-03-05T08:56:01Z

Hi @ltagliamonte-dd , sorry for late response.

This PR includes massive changes to existing, critical code (the encoding logic of HASH data structure). So it requires lots of time to review to ensure that, not only users that need this feature, but also users that don't care this feature, can have a good and consistent experience while using Kvrocks. Also we need to make sure it doesn't affect old users, and the performance doesn't have a big drop.

I currently don't have much time to complete a full review due to some personal affairs. Not sure if @git-hulk and @mapleFU have time to review. But if I get some time I'll move on for this PR : )

From the implementation, it should not affect the user who doesn't use the hash expiration feature. But the performance would be heavily impacted once enabled, because it needs to count the valid field number at each operation. And it would be hard to improve once we have released this, so I have no confidence to say it's fine to have a try from my perspective. But I admit the hash expiration is a key feature for Kvrocks and desire to introduce this feature.

ltagliamonte · 2025-03-21T13:36:12Z

thanks @git-hulk , what do you prefer to do? back to the drawing board?
I'm happy to contribute if we find a better way to add this feature.

torwig · 2025-03-21T14:04:27Z

I'm not sure, but maybe Redis (according to this PR, I'll try to double check) tracks the latest expiration time of the element in the hash so we can save it to Metadata and take it into consideration without checking all valid elements in the hash. So this solution will bring the same performance to both "normal hash" and the "hash with elements that have TTL". @git-hulk @PragmaTwice @mapleFU WDYT?

salarali · 2025-05-30T19:54:15Z

Any updates or progress on this feature?

filefox · 2025-09-09T03:19:03Z

need this feature.

jjz921024 and others added 4 commits December 20, 2024 18:09

Support hash field expiration

2208cdb

revert changes to GetMetadata

e743f04

fix CommandHStrlen

a1afdc5

Merge branch 'apache:unstable' into hfe

88ef4ca

ltagliamonte-dd changed the title ~~Hfe~~ feat(hash): Support hash field expiration Jan 9, 2025

ltagliamonte-dd and others added 2 commits January 8, 2025 18:36

fix typo

201c226

Merge branch 'unstable' into hfe

f6d0cb7

fix unit

d3a2b74

Merge branch 'unstable' into hfe

45a864a

git-hulk requested review from PragmaTwice, caipengbo, git-hulk, mapleFU and torwig January 11, 2025 04:54

git-hulk changed the title ~~feat(hash): Support hash field expiration~~ feat(hash): add support of hash expiration commands Jan 11, 2025

jjz921024 mentioned this pull request Jan 11, 2025

feat(hash): Support hash field expiration #2402

Open

ltagliamonte-dd and others added 5 commits January 13, 2025 09:41

Merge branch 'unstable' into hfe

fccc2fe

add GetValidFieldCount method

9de4b52

make the error msg consistent

4b87842

fix msg and use new func

b9aeaba

empty commit to re trigger CI

a6a07bb

ltagliamonte-dd added 2 commits January 13, 2025 18:13

fix dereference

4240628

remove unused var

dbd9d13

git-hulk added 2 commits January 19, 2025 09:58

Merge branch 'unstable' into hfe

35ce911

Merge branch 'unstable' into hfe

3eefb37

Merge branch 'unstable' into hfe

f0938a1

PragmaTwice reviewed Jan 25, 2025

View reviewed changes

kvrocks.conf Outdated Show resolved Hide resolved

PragmaTwice reviewed Jan 25, 2025

View reviewed changes

ltagliamonte-dd and others added 6 commits January 25, 2025 13:27

Update kvrocks.conf

c5ec9f0

Co-authored-by: Twice <twice@apache.org>

address PragmaTwice feedbacks

3a726e1

use string_view

b10d710

Merge branch 'unstable' into hfe

f937ec8

address last comments

9bbc8d9

Merge branch 'unstable' into hfe

009fca3

ltagliamonte-dd and others added 2 commits January 28, 2025 08:23

Merge branch 'unstable' into hfe

6ebc2c8

Merge remote-tracking branch 'origin/unstable' into hfe

1a45388

Merge branch 'unstable' into hfe

581c1f4

This was referenced Jan 22, 2026

feat(hash): add per-field expiration support dashjay/kvrocks#1

Open

feat(hash): add per-field expiration support #3346

Open

Conversation

ltagliamonte-dd commented Jan 9, 2025

Uh oh!

jjz921024 commented Jan 9, 2025

Uh oh!

ltagliamonte-dd commented Jan 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ltagliamonte commented Jan 10, 2025

Uh oh!

git-hulk commented Jan 11, 2025

Uh oh!

jjz921024 commented Jan 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ltagliamonte-dd commented Jan 13, 2025

Uh oh!

ltagliamonte-dd commented Jan 13, 2025

Uh oh!

PragmaTwice commented Jan 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ltagliamonte-dd commented Jan 22, 2025

Uh oh!

Uh oh!

PragmaTwice Jan 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ltagliamonte-dd Jan 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PragmaTwice Jan 25, 2025

Choose a reason for hiding this comment

Uh oh!

ltagliamonte-dd Jan 25, 2025

Choose a reason for hiding this comment

Uh oh!

ltagliamonte-dd Jan 25, 2025

Choose a reason for hiding this comment

Uh oh!

PragmaTwice Jan 25, 2025

Choose a reason for hiding this comment

Uh oh!

PragmaTwice Jan 25, 2025

Choose a reason for hiding this comment

Uh oh!

ltagliamonte-dd Jan 25, 2025

Choose a reason for hiding this comment

Uh oh!

PragmaTwice left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

git-hulk commented Jan 25, 2025

Uh oh!

sonarqubecloud bot commented Jan 26, 2025

Quality Gate passed

Uh oh!

ltagliamonte-dd commented Feb 10, 2025

Uh oh!

ltagliamonte-dd commented Mar 4, 2025

Uh oh!

sonarqubecloud bot commented Mar 4, 2025

Quality Gate passed

Uh oh!

PragmaTwice commented Mar 5, 2025

Uh oh!

git-hulk commented Mar 5, 2025

Uh oh!

ltagliamonte commented Mar 21, 2025

Uh oh!

torwig commented Mar 21, 2025

Uh oh!

salarali commented May 30, 2025

Uh oh!

filefox commented Sep 9, 2025

Uh oh!

Reviewers

ltagliamonte-dd commented Jan 9, 2025 •

edited

Loading

jjz921024 commented Jan 12, 2025 •

edited

Loading

PragmaTwice commented Jan 22, 2025 •

edited

Loading

PragmaTwice Jan 25, 2025 •

edited

Loading

ltagliamonte-dd Jan 25, 2025 •

edited

Loading

PragmaTwice left a comment •

edited

Loading