You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by "PragmaTwice (via GitHub)" <gi...@apache.org> on 2026/09/19 07:51:01 UTC
[PR] feat(hash): support HFE in WAL-to-RESP conversion [kvrocks]
PragmaTwice opened a new pull request, #3634:
URL: https://github.com/apache/kvrocks/pull/3634
Adds hash encoding context to WAL and preserves field expiration when converting hash writes and COPY/RENAME to RESP.
Refs #3436; migration batching and kvrocks2redis full export are left for follow-up PRs.
Assisted-by: Codex
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(hash): support HFE in WAL-to-RESP conversion [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #3634:
URL: https://github.com/apache/kvrocks/pull/3634#discussion_r4070067097
##########
src/storage/batch_extractor.cc:
##########
@@ -133,9 +133,23 @@ rocksdb::Status WriteBatchExtractor::PutCF(uint32_t column_family_id, const Slic
ns = ikey.GetNamespace().ToString();
switch (log_data_.GetRedisType()) {
- case kRedisHash:
- command_args = {"HSET", user_key, sub_key, value.ToString()};
- break;
+ case kRedisHash: {
Review Comment:
HFE has not been released yet. So there's no old HFE WAL.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(hash): support HFE in WAL-to-RESP conversion [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #3634:
URL: https://github.com/apache/kvrocks/pull/3634#discussion_r4052727161
##########
src/storage/batch_extractor.cc:
##########
@@ -133,9 +133,23 @@ rocksdb::Status WriteBatchExtractor::PutCF(uint32_t column_family_id, const Slic
ns = ikey.GetNamespace().ToString();
switch (log_data_.GetRedisType()) {
- case kRedisHash:
- command_args = {"HSET", user_key, sub_key, value.ToString()};
- break;
+ case kRedisHash: {
+ auto mode = log_data_.GetHashSubkeyEncodingMode();
+ if (!mode) return rocksdb::Status::InvalidArgument(mode.Msg());
+ HashMetadata metadata(false, *mode);
+ Slice field_value = value;
+ uint64_t expire = 0;
+ auto s = metadata.DecodeSubkeyValue(&field_value, &expire);
+ if (!s.ok()) return s;
+
+ resp_commands_[ns].emplace_back(redis::ArrayOfBulkStrings({"HSET", user_key, sub_key, field_value.ToString()}));
+ // Replay expired writes too: the destination may still contain an older field value.
+ if (expire > 0) {
+ resp_commands_[ns].emplace_back(
+ redis::ArrayOfBulkStrings({"HPEXPIREAT", user_key, std::to_string(expire), "FIELDS", "1", sub_key}));
+ }
Review Comment:
HSET && HPEXPIREAT: requires Redis >= 7.4
HSETEX: requires Redis >= 8.0
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(hash): support HFE in WAL-to-RESP conversion [kvrocks]
Posted by "jihuayu (via GitHub)" <gi...@apache.org>.
jihuayu commented on code in PR #3634:
URL: https://github.com/apache/kvrocks/pull/3634#discussion_r4070223708
##########
src/storage/batch_extractor.cc:
##########
@@ -133,9 +133,23 @@ rocksdb::Status WriteBatchExtractor::PutCF(uint32_t column_family_id, const Slic
ns = ikey.GetNamespace().ToString();
switch (log_data_.GetRedisType()) {
- case kRedisHash:
- command_args = {"HSET", user_key, sub_key, value.ToString()};
- break;
+ case kRedisHash: {
Review Comment:
OK!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(hash): support HFE in WAL-to-RESP conversion [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #3634:
URL: https://github.com/apache/kvrocks/pull/3634#discussion_r4070186854
##########
src/storage/batch_extractor.cc:
##########
@@ -133,9 +133,23 @@ rocksdb::Status WriteBatchExtractor::PutCF(uint32_t column_family_id, const Slic
ns = ikey.GetNamespace().ToString();
switch (log_data_.GetRedisType()) {
- case kRedisHash:
- command_args = {"HSET", user_key, sub_key, value.ToString()};
- break;
+ case kRedisHash: {
Review Comment:
Yah it is considered. You can see we have `log_data_.GetHashSubkeyEncodingMode()`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(hash): support HFE in WAL-to-RESP conversion [kvrocks]
Posted by "jihuayu (via GitHub)" <gi...@apache.org>.
jihuayu commented on code in PR #3634:
URL: https://github.com/apache/kvrocks/pull/3634#discussion_r4070091355
##########
src/storage/batch_extractor.cc:
##########
@@ -133,9 +133,23 @@ rocksdb::Status WriteBatchExtractor::PutCF(uint32_t column_family_id, const Slic
ns = ikey.GetNamespace().ToString();
switch (log_data_.GetRedisType()) {
- case kRedisHash:
- command_args = {"HSET", user_key, sub_key, value.ToString()};
- break;
+ case kRedisHash: {
Review Comment:
Ohh，I mean old Hash Set WAL. It also used `kRedisHash` RedisType. @PragmaTwice
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(hash): support HFE in WAL-to-RESP conversion [kvrocks]
Posted by "jihuayu (via GitHub)" <gi...@apache.org>.
jihuayu merged PR #3634:
URL: https://github.com/apache/kvrocks/pull/3634
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(hash): support HFE in WAL-to-RESP conversion [kvrocks]
Posted by "jihuayu (via GitHub)" <gi...@apache.org>.
jihuayu commented on code in PR #3634:
URL: https://github.com/apache/kvrocks/pull/3634#discussion_r4069534337
##########
src/storage/batch_extractor.cc:
##########
@@ -133,9 +133,23 @@ rocksdb::Status WriteBatchExtractor::PutCF(uint32_t column_family_id, const Slic
ns = ikey.GetNamespace().ToString();
switch (log_data_.GetRedisType()) {
- case kRedisHash:
- command_args = {"HSET", user_key, sub_key, value.ToString()};
- break;
+ case kRedisHash: {
Review Comment:
Can we make sure the new version will never read WAL from an older version? The current code cannot tell old WAL from new WAL. If it reads an old HFE WAL, it may silently write wrong data. Is this too risky?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org