feat(hash): add per-field expiration support (HEXPIRE, HTTL, HPERSIST)#3269
feat(hash): add per-field expiration support (HEXPIRE, HTTL, HPERSIST)#3269tejaslodayadd wants to merge 28 commits into
Conversation
|
@torwig could you please check this PR, as I remember, we talk about this feature? Thanks |
|
@tejaslodayadd lot ot thanks, cool feature, we need support of this |
| // Check for hash field expiration | ||
| if (metadata.Type() == kRedisHash) { | ||
| // First check if metadata (key-level) is expired | ||
| if (IsMetadataExpired(ikey, metadata)) { | ||
| return true; | ||
| } | ||
| // Then check per-field expiration | ||
| // Use lazy deletion with 5 minute buffer similar to metadata expiration | ||
| uint64_t lazy_expired_ts = util::GetTimeStampMS() - 300000; | ||
| HashFieldValue field_value; | ||
| if (!HashFieldValue::Decode(value.ToString(), &field_value)) { | ||
| // Failed to decode, keep the field | ||
| return false; | ||
| } | ||
| // Check if field has expiration and if it's expired (with lazy delete buffer) | ||
| if (field_value.expire > 0 && field_value.expire <= lazy_expired_ts) { | ||
| return true; | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
The key logic for hash field expiration cleanup during compaction -- when the metadata type is kRedisHash, the filter:
- First checks if the entire key is expired (key-level expiration)
- Then decodes the field value using
HashFieldValue::Decode() - Checks if the field has a non-zero expiration timestamp
- Uses a 5-minute lazy delete buffer (
util::GetTimeStampMS() - 300000) to avoid race conditions between the HEXPIRE command and compaction (same pattern used for key-level expiration)
|
After discussing this internally, there's a problem: Currently,
Options:
Let us know which way you'd want us to move forward @PragmaTwice @torwig @aleksraiden |
|
@tejaslodayadd Thanks for your contribution. I just had a glance at this PR and have two questions about the design:
From my personal perspective, it's acceptable to sacrifice the short-term correctness of a command like HLEN to improve performance. |
|
Hi, thank you for your contribution. I skimmed through and found some issue in this design:
|
|
@git-hulk @PragmaTwice FYI I'm going to help @tejaslodayadd with this PR. I'm currently debating how to proceed with the compaction size/problem.
What do you folks think about this approach? |
|
when it comes to: i've implemented a new format that uses a two-byte magic marker (0xFF 0xFE) instead of a single byte, let me know if this works for you @PragmaTwice New format: [[0xFF][0xFE][1-byte flags][8-byte timestamp if flag set][value]] |
|
@PragmaTwice @git-hulk Just pushed the lazy repair approach discussed before, looking forward to your feedbacks! |
| subkey_opts.disable_auto_compactions = config_->rocks_db.disable_auto_compactions; | ||
| subkey_opts.table_properties_collector_factories.emplace_back( | ||
| NewCompactOnExpiredTableCollectorFactory(std::string(kPrimarySubkeyColumnFamilyName), 0.3)); | ||
| subkey_opts.merge_operator = std::make_shared<HashMergeOperator>(); |
There was a problem hiding this comment.
The merge operator would be invoked when iterating over any keys in this column family, and it could significantly affect performance.
| return rocksdb::Status::OK(); | ||
| } | ||
|
|
||
| void Hash::asyncRepairHash(const std::string &ns_key, const Slice &field, const HashMetadata &metadata) const { |
There was a problem hiding this comment.
It's not a good idea to spawn a thread per expired hash field. Perhaps we can reuse the existing task runner, or have a fixed thread pool for this purpose.
|
|
@PragmaTwice @git-hulk would love to hear from you as well, in order to focus the development in the right direction. |
|
@PragmaTwice @git-hulk @aleksraiden gentle ping about my last few comments. |
|
@PragmaTwice @git-hulk @aleksraiden gentle ping about my last few comments. thanks |
|
Still wrong clang-tidy check as i see. @torwig could you check and suggest? Lot of thanks |
|
@aleksraiden thanks yes that needs to be addressed, but i would love to hear from the community leaders how to proceed with this feature.
basically after the first major compaction all hash keys would be migrated to v2. |
Mrt3383x38
left a comment
There was a problem hiding this comment.
Selam ugulama güncellendigi zaman ne borsada binance nede maht wallet çalışmıyo tsk.
|
@ltagliamonte-dd I tried to make another PR, PTLA, go easy on me 👍 |
|
@PragmaTwice @git-hulk @aleksraiden gentle ping. Please let me and @ltagliamonte-dd know if you could review this PR |
|
Please, check an error at tests |
|
@PragmaTwice @git-hulk @aleksraiden I updated the PR. Please let me and @ltagliamonte-dd know if you could review this PR |
|
I recall we had an online discussion regarding this feature. Was the outcome of that discussion shared with the community? |
Made-with: Cursor
I'm not sure about this. @ltagliamonte-dd if you recall ? |
@thailowki @jihuayu i've tried to reach out @git-hulk @aleksraiden @PragmaTwice on zulipchat but didn't get any attention. |
|
As I see, at now a main stopper for update and test are unresolved conflicts:
P.S. Sorry, I'am only in this place, no possibility to answer at zulip chat, sorry. |
|
Refer to #3436. |
Implements per-field expiration support for HASH data type, following Redis 7.4 specification.
Commands:
Implementation Details
[raw value][0xFF][flags][8-byte timestamp][value]This extends the existing key-level expiration (
HSETEXPIRE) added in #2750 to support per-field granularity, which is a common feature request similar to Redis 7.4's hash field expiration support.Reference: https://redis.io/docs/latest/develop/data-types/hashes/