feat(hash): add support of hash expiration commands #2717

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.

git-hulk · 2025-01-15T02:03:16Z

@PragmaTwice @mapleFU @torwig @caipengbo To see if you guys can have a look at this PR. It generally looks good to me. It'd be nice to push this feature forward to avoid pending too long time.

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 <[email protected]>

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

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, git-hulk, torwig, caipengbo and mapleFU 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

remove empty line

67f41c3

git-hulk added 3 commits January 17, 2025 18:51

Merge branch 'unstable' into hfe

cf0ff18

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 <[email protected]>

address PragmaTwice feedbacks

3a726e1

use string_view

b10d710

Merge branch 'unstable' into hfe

f937ec8

address last comments

9bbc8d9

Merge branch 'unstable' into hfe

009fca3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(hash): add support of hash expiration commands #2717

feat(hash): add support of hash expiration commands #2717

ltagliamonte-dd commented Jan 9, 2025

jjz921024 commented Jan 9, 2025

ltagliamonte-dd commented Jan 9, 2025 •

edited

Loading

ltagliamonte commented Jan 10, 2025

git-hulk commented Jan 11, 2025

jjz921024 commented Jan 12, 2025 •

edited

Loading

ltagliamonte-dd commented Jan 13, 2025

ltagliamonte-dd commented Jan 13, 2025

git-hulk commented Jan 15, 2025

PragmaTwice commented Jan 22, 2025 •

edited

Loading

ltagliamonte-dd commented Jan 22, 2025

PragmaTwice Jan 25, 2025 •

edited

Loading

ltagliamonte-dd Jan 25, 2025 •

edited

Loading

PragmaTwice Jan 25, 2025

ltagliamonte-dd Jan 25, 2025

ltagliamonte-dd Jan 25, 2025

PragmaTwice Jan 25, 2025

PragmaTwice Jan 25, 2025

ltagliamonte-dd Jan 25, 2025

PragmaTwice left a comment •

edited

Loading

git-hulk commented Jan 25, 2025

sonarqubecloud bot commented Jan 26, 2025

feat(hash): add support of hash expiration commands #2717

Are you sure you want to change the base?

feat(hash): add support of hash expiration commands #2717

Conversation

ltagliamonte-dd commented Jan 9, 2025

jjz921024 commented Jan 9, 2025

ltagliamonte-dd commented Jan 9, 2025 • edited Loading

ltagliamonte commented Jan 10, 2025

git-hulk commented Jan 11, 2025

jjz921024 commented Jan 12, 2025 • edited Loading

ltagliamonte-dd commented Jan 13, 2025

ltagliamonte-dd commented Jan 13, 2025

git-hulk commented Jan 15, 2025

PragmaTwice commented Jan 22, 2025 • edited Loading

ltagliamonte-dd commented Jan 22, 2025

PragmaTwice Jan 25, 2025 • edited Loading

Choose a reason for hiding this comment

ltagliamonte-dd Jan 25, 2025 • edited Loading

Choose a reason for hiding this comment

PragmaTwice Jan 25, 2025

Choose a reason for hiding this comment

ltagliamonte-dd Jan 25, 2025

Choose a reason for hiding this comment

ltagliamonte-dd Jan 25, 2025

Choose a reason for hiding this comment

PragmaTwice Jan 25, 2025

Choose a reason for hiding this comment

PragmaTwice Jan 25, 2025

Choose a reason for hiding this comment

ltagliamonte-dd Jan 25, 2025

Choose a reason for hiding this comment

PragmaTwice left a comment • edited Loading

Choose a reason for hiding this comment

git-hulk commented Jan 25, 2025

sonarqubecloud bot commented Jan 26, 2025

Quality Gate passed

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