feat(hash): add support of hash expiration commands

[ltagliamonte-dd]

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]

@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]

@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]

@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]

@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:

1. Change ExistValidField to GetValidFieldCount, so that Hash::Size can also resue this function.
2. 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]

@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]

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]

@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]

@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.

[sonarqubecloud]

Quality Gate passed

Issues
40 New issues
0 Accepted issues

Measures
0 Security Hotspots
69.4% Coverage on New Code
14.6% Duplication on New Code

See analysis details on SonarQube Cloud