mapleFU commented on code in PR #2389:
URL: https://github.com/apache/kvrocks/pull/2389#discussion_r1676798810
##########
src/storage/batch_extractor.cc:
##########
@@ -279,15 +281,17 @@ rocksdb::Status WriteBatchExtractor::DeleteCF(uint32_t
column_family_id, const S
std::string user_key;
std::tie(ns, user_key) = ExtractNamespaceKey<std::string>(key,
is_slot_id_encoded_);
- if (slot_id_ >= 0 && static_cast<uint16_t>(slot_id_) !=
GetSlotIdFromKey(user_key)) {
+ auto key_slot_id = GetSlotIdFromKey(user_key);
+ if (slot_range_.IsValid() && !slot_range_.Contains(key_slot_id)) {
Review Comment:
Is `Contains` enough here?
##########
src/cluster/cluster_defs.h:
##########
@@ -33,11 +34,34 @@ enum {
inline constexpr const char *errInvalidNodeID = "Invalid cluster node id";
inline constexpr const char *errInvalidSlotID = "Invalid slot id";
inline constexpr const char *errSlotOutOfRange = "Slot is out of range";
+inline constexpr const char *errSlotRangeInvalid = "Slot range is invalid";
inline constexpr const char *errInvalidClusterVersion = "Invalid cluster
version";
inline constexpr const char *errSlotOverlapped = "Slot distribution is
overlapped";
inline constexpr const char *errNoMasterNode = "The node isn't a master";
inline constexpr const char *errClusterNoInitialized = "The cluster is not
initialized";
inline constexpr const char *errInvalidClusterNodeInfo = "Invalid cluster
nodes info";
inline constexpr const char *errInvalidImportState = "Invalid import state";
-using SlotRange = std::pair<int, int>;
+struct SlotRange {
Review Comment:
Perhaps a comment for inclusive is better here
##########
src/storage/batch_extractor.cc:
##########
@@ -279,15 +281,17 @@ rocksdb::Status WriteBatchExtractor::DeleteCF(uint32_t
column_family_id, const S
std::string user_key;
std::tie(ns, user_key) = ExtractNamespaceKey<std::string>(key,
is_slot_id_encoded_);
- if (slot_id_ >= 0 && static_cast<uint16_t>(slot_id_) !=
GetSlotIdFromKey(user_key)) {
+ auto key_slot_id = GetSlotIdFromKey(user_key);
+ if (slot_range_.IsValid() && !slot_range_.Contains(key_slot_id)) {
return rocksdb::Status::OK();
}
command_args = {"DEL", user_key};
} else if (column_family_id ==
static_cast<uint32_t>(ColumnFamilyID::PrimarySubkey)) {
InternalKey ikey(key, is_slot_id_encoded_);
std::string user_key = ikey.GetKey().ToString();
- if (slot_id_ >= 0 && static_cast<uint16_t>(slot_id_) !=
GetSlotIdFromKey(user_key)) {
+ auto key_slot_id = GetSlotIdFromKey(user_key);
+ if (slot_range_.IsValid() && !slot_range_.Contains(key_slot_id)) {
Review Comment:
ditto?
##########
src/cluster/cluster.cc:
##########
@@ -214,7 +214,7 @@ Status Cluster::SetClusterNodes(const std::string
&nodes_str, int64_t version, b
if (!migrated_slots_.empty()) {
for (const auto &[slot, _] : migrated_slots_) {
if (slots_nodes_[slot] != myself_) {
- auto s = srv_->slot_migrator->ClearKeysOfSlot(kDefaultNamespace, slot);
+ auto s = srv_->slot_migrator->ClearKeysOfSlotRange(kDefaultNamespace,
{slot, slot});
Review Comment:
Nit: a helper function for single range is ok here
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
