-
Notifications
You must be signed in to change notification settings - Fork 591
feat(hash): add per-field expiration support (HEXPIRE, HTTL, HPERSIST) #3269
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: unstable
Are you sure you want to change the base?
feat(hash): add per-field expiration support (HEXPIRE, HTTL, HPERSIST) #3269
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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! |
src/storage/storage.cc
Outdated
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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/