You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by "git-hulk (via GitHub)" <gi...@apache.org> on 2023/03/22 16:21:24 UTC
[GitHub] [incubator-kvrocks] git-hulk opened a new pull request, #1345: Fix missing stream increment batch in cluster migration
git-hulk opened a new pull request, #1345:
URL: https://github.com/apache/incubator-kvrocks/pull/1345
Currently, the cluster slot migration will split into two stages: send snapshots and sync WAL if the snapshot was sent. For the stream, we only parsed the KVs from the snapshot and missed the WAL part.
--
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
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #1345: Fix missing stream increment batch in cluster migration
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #1345:
URL: https://github.com/apache/incubator-kvrocks/pull/1345#discussion_r1145679599
##########
src/cluster/slot_migrate.cc:
##########
@@ -765,23 +765,10 @@ Status SlotMigrate::MigrateStream(const Slice &key, const StreamMetadata &metada
break;
}
- // Parse values of the complex key
- // InternalKey is adopted to get complex key's value from the formatted key returned by iterator of rocksdb
- InternalKey inkey(iter->key(), true);
- std::vector<std::string> values;
- auto s = Redis::DecodeRawStreamEntryValue(iter->value().ToString(), &values);
+ auto s = WriteBatchExtractor::ExtractStreamAddCommand(true, iter->key(), iter->value(), &user_cmd);
if (!s.IsOK()) {
- return {Status::NotOK, fmt::format("failed to decode stream values: {}", s.Msg())};
+ return s;
Review Comment:
```suggestion
return s.Prefixed("failed to decode stream values");
```
##########
src/storage/batch_extractor.cc:
##########
@@ -291,3 +302,23 @@ rocksdb::Status WriteBatchExtractor::DeleteRangeCF(uint32_t column_family_id, co
// Do nothing about DeleteRange operations
return rocksdb::Status::OK();
}
+
+Status WriteBatchExtractor::ExtractStreamAddCommand(bool is_slotid_encoded, const Slice &subkey, const Slice &value,
+ std::vector<std::string> *command_args) {
+ InternalKey ikey(subkey, is_slotid_encoded);
+ std::string user_key = ikey.GetKey().ToString();
+ *command_args = {"XADD", user_key};
+ std::vector<std::string> values;
+ auto s = Redis::DecodeRawStreamEntryValue(value.ToString(), &values);
+ if (!s.IsOK()) {
+ return {Status::NotOK, fmt::format("failed to decode stream values: {}", s.Msg())};
Review Comment:
```suggestion
return s.Prefixed("failed to decode stream values");
```
--
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
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #1345: Fix missing stream increment batch in cluster migration
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #1345:
URL: https://github.com/apache/incubator-kvrocks/pull/1345#discussion_r1145679599
##########
src/cluster/slot_migrate.cc:
##########
@@ -765,23 +765,10 @@ Status SlotMigrate::MigrateStream(const Slice &key, const StreamMetadata &metada
break;
}
- // Parse values of the complex key
- // InternalKey is adopted to get complex key's value from the formatted key returned by iterator of rocksdb
- InternalKey inkey(iter->key(), true);
- std::vector<std::string> values;
- auto s = Redis::DecodeRawStreamEntryValue(iter->value().ToString(), &values);
+ auto s = WriteBatchExtractor::ExtractStreamAddCommand(true, iter->key(), iter->value(), &user_cmd);
if (!s.IsOK()) {
- return {Status::NotOK, fmt::format("failed to decode stream values: {}", s.Msg())};
+ return s;
Review Comment:
```suggestion
return s.Prefixed("failed to decode stream values");
```
--
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
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #1345: Fix missing stream increment batch in cluster migration
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on PR #1345:
URL: https://github.com/apache/incubator-kvrocks/pull/1345#issuecomment-1479887155
@torwig I'm not quite familiar with the XSETID command, so I didn't handle it now.
--
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
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #1345: Fix missing stream increment batch in cluster migration
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on PR #1345:
URL: https://github.com/apache/incubator-kvrocks/pull/1345#issuecomment-1482280964
Thanks all, merging..
--
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
[GitHub] [incubator-kvrocks] torwig commented on pull request #1345: Fix missing stream increment batch in cluster migration
Posted by "torwig (via GitHub)" <gi...@apache.org>.
torwig commented on PR #1345:
URL: https://github.com/apache/incubator-kvrocks/pull/1345#issuecomment-1479898875
@git-hulk Thanks for your work. `SETID` is intended for things like replication (I can say it is an internal command) to set the values of last_added_id, entries_added, max_deleted_id. I used it as the last command in slot migration to update the stream's metadata, because with the `XADD` you can't migrate last_added_id, entries_added and max_deleted_id, because it actually just adds entries but not deletes them.
--
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
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #1345: Fix missing stream increment batch in cluster migration
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on PR #1345:
URL: https://github.com/apache/incubator-kvrocks/pull/1345#issuecomment-1480493926
@torwig Thank you. So for the XSETID command, I should parse the last_added_id / entries_added / max_deleted_id, then translate it into the XSETID. Is it right?
--
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
[GitHub] [incubator-kvrocks] torwig commented on pull request #1345: Fix missing stream increment batch in cluster migration
Posted by "torwig (via GitHub)" <gi...@apache.org>.
torwig commented on PR #1345:
URL: https://github.com/apache/incubator-kvrocks/pull/1345#issuecomment-1480599883
@git-hulk Yes, absolutely.
--
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
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #1345: Fix missing stream increment batch in cluster migration
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on PR #1345:
URL: https://github.com/apache/incubator-kvrocks/pull/1345#issuecomment-1480543305
@ShooterIT Yes, it's finished but missed the WAL's increment sync.
--
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
[GitHub] [incubator-kvrocks] ShooterIT commented on pull request #1345: Fix missing stream increment batch in cluster migration
Posted by "ShooterIT (via GitHub)" <gi...@apache.org>.
ShooterIT commented on PR #1345:
URL: https://github.com/apache/incubator-kvrocks/pull/1345#issuecomment-1480537561
BTW, do we implement https://github.com/apache/incubator-kvrocks/pull/577#issuecomment-1128483342?
--
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
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #1345: Fix missing stream increment batch in cluster migration
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on code in PR #1345:
URL: https://github.com/apache/incubator-kvrocks/pull/1345#discussion_r1147050397
##########
tests/gocase/integration/slotmigrate/slotmigrate_test.go:
##########
@@ -542,6 +542,45 @@ func TestSlotMigrateDataType(t *testing.T) {
}
})
+ t.Run("MIGRATE - increment sync stream from WAL", func(t *testing.T) {
+ slot := 3
Review Comment:
```suggestion
slot := 40
```
--
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
[GitHub] [incubator-kvrocks] git-hulk merged pull request #1345: Fix missing stream increment batch in cluster migration
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk merged PR #1345:
URL: https://github.com/apache/incubator-kvrocks/pull/1345
--
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