Copilot commented on code in PR #3564:
URL: https://github.com/apache/kvrocks/pull/3564#discussion_r3620945813
##########
src/storage/batch_extractor.cc:
##########
@@ -408,10 +412,42 @@ rocksdb::Status WriteBatchExtractor::DeleteCF(uint32_t
column_family_id, const S
return rocksdb::Status::OK();
}
-rocksdb::Status WriteBatchExtractor::DeleteRangeCF([[maybe_unused]] uint32_t
column_family_id,
- [[maybe_unused]] const
Slice &begin_key,
- [[maybe_unused]] const
Slice &end_key) {
- // Do nothing with DeleteRange operations
+rocksdb::Status WriteBatchExtractor::DeleteRangeCF(uint32_t column_family_id,
const Slice &begin_key,
+ const Slice &end_key) {
+ if (column_family_id != static_cast<uint32_t>(ColumnFamilyID::Metadata)) {
+ return rocksdb::Status::OK();
+ }
+
+ auto check_and_return_namespace = [](const Slice &begin_key, const Slice
&end_key) -> std::optional<std::string> {
+ if (begin_key.empty()) {
+ return std::nullopt;
+ }
+
+ auto namespace_size = static_cast<uint8_t>(begin_key.data()[0]);
+ if (begin_key.size() != sizeof(uint8_t) + namespace_size) {
+ return std::nullopt;
+ }
+
+ std::string ns = begin_key.ToString().substr(sizeof(uint8_t),
namespace_size);
+ // Redis has no range-delete command;
+ // only a range covering an entire namespace can be translated into
FLUSHDB.
+ std::string expected_begin = ComposeNamespaceKey(ns, "",
/*slot_id_encoded=*/false);
+ std::string expected_end = util::StringNext(expected_begin);
+ if (begin_key.ToString() != expected_begin || end_key.ToString() !=
expected_end) {
+ return std::nullopt;
+ }
+
+ return ns;
+ };
+
+ auto ns = check_and_return_namespace(begin_key, end_key);
+ if (!ns.has_value()) {
+ WARN("Rejecting unrecognized DeleteRange in metadata CF, begin_key={},
end_key={}",
+ util::StringToHex(begin_key.ToString()),
util::StringToHex(end_key.ToString()));
+ return rocksdb::Status::NotSupported("unrecognized DeleteRange in metadata
CF");
+ }
Review Comment:
Returning NotSupported for unrecognized DeleteRange in the metadata CF will
make CommandPollUpdates (FORMAT RESP) fail the entire batch translation
(cmd_server.cc checks Iterate() status and returns RedisExecErr). This can be
triggered by other metadata DeleteRange users, e.g.
Database::ClearKeysOfSlotRange() issues a slot-prefix DeleteRange in the
metadata CF, which this validator will reject. Prefer logging a warning but
returning OK so polling/kvrocks2redis remains best-effort and doesn’t break on
unrelated range deletes.
##########
tests/cppunit/batch_extractor_test.cc:
##########
@@ -0,0 +1,151 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#include "storage/batch_extractor.h"
+
+#include <gtest/gtest.h>
+#include <rocksdb/write_batch.h>
+
+#include "common/string_util.h"
+#include "config/config.h"
+#include "server/redis_reply.h"
+#include "test_base.h"
+#include "types/redis_string.h"
+
+namespace {
+
+std::vector<std::string> GetCommands(WriteBatchExtractor *extractor, const
std::string &ns) {
+ auto *commands = extractor->GetRESPCommands();
+ if (auto it = commands->find(ns); it != commands->end()) {
+ return it->second;
+ }
+ return {};
+}
+
+} // namespace
+
+TEST(WriteBatchExtractorTest, ExtractFlushDBFromNamespaceDeleteRange) {
+ WriteBatchExtractor extractor(false, -1, true);
+ auto begin_key = ComposeNamespaceKey(kDefaultNamespace, "", false);
+ auto end_key = util::StringNext(begin_key);
+
+ auto s =
extractor.DeleteRangeCF(static_cast<uint32_t>(ColumnFamilyID::Metadata),
begin_key, end_key);
+ ASSERT_TRUE(s.ok()) << s.ToString();
+
+ auto commands = GetCommands(&extractor, kDefaultNamespace);
+ ASSERT_EQ(commands.size(), 1);
+ EXPECT_EQ(commands[0], redis::ArrayOfBulkStrings({"FLUSHDB"}));
+}
+
+TEST(WriteBatchExtractorTest,
ExtractFlushDBFromNonDefaultNamespaceDeleteRange) {
+ std::string ns = "test-ns";
+ WriteBatchExtractor extractor(false, -1, true);
+ auto begin_key = ComposeNamespaceKey(ns, "", false);
+ auto end_key = util::StringNext(begin_key);
+
+ auto s =
extractor.DeleteRangeCF(static_cast<uint32_t>(ColumnFamilyID::Metadata),
begin_key, end_key);
+ ASSERT_TRUE(s.ok()) << s.ToString();
+
+ auto commands = GetCommands(&extractor, ns);
+ ASSERT_EQ(commands.size(), 1);
+ EXPECT_EQ(commands[0], redis::ArrayOfBulkStrings({"FLUSHDB"}));
+}
+
+TEST(WriteBatchExtractorTest, RejectNonFlushDBDeleteRange) {
+ std::string ns = "test-ns";
+ WriteBatchExtractor extractor(false, -1, true);
+ auto begin_key = ComposeNamespaceKey(ns, "key", false);
+ auto end_key = util::StringNext(begin_key);
+
+ auto s =
extractor.DeleteRangeCF(static_cast<uint32_t>(ColumnFamilyID::Metadata),
begin_key, end_key);
+ ASSERT_TRUE(s.IsNotSupported()) << s.ToString();
+
+ EXPECT_TRUE(extractor.GetRESPCommands()->empty());
Review Comment:
These tests lock in the behavior of treating non-FLUSH DeleteRange in the
metadata CF as NotSupported. If DeleteRangeCF is changed to ‘warn + ignore +
return OK’ (to avoid breaking POLLUPDATES/kvrocks2redis on other metadata range
deletes), update the assertions here to expect OK while still asserting that no
RESP commands were produced.
##########
src/storage/redis_db.cc:
##########
@@ -458,17 +459,27 @@ rocksdb::Status Database::FlushDB(engine::Context &ctx) {
rocksdb::Status Database::FlushAll(engine::Context &ctx) {
auto iter = util::UniqueIterator(ctx, ctx.GetReadOptions(),
metadata_cf_handle_);
- iter->SeekToFirst();
- if (!iter->Valid()) {
- return rocksdb::Status::OK();
+ std::set<std::string> namespaces;
+ for (iter->SeekToFirst(); iter->Valid(); iter->Next()) {
+ auto [ns_slice, _] = ExtractNamespaceKey(iter->key(),
storage_->IsSlotIdEncoded());
+ namespaces.emplace(ns_slice.ToString());
+ }
Review Comment:
FlushAll now scans *every* key in the metadata column family to collect
namespaces, which is O(number_of_keys) and a regression from the prior
SeekToFirst/SeekToLast approach. Since metadata keys are ordered by namespace
prefix, you can skip to the next namespace by seeking to
StringNext(ComposeNamespaceKey(ns, "", false)) each iteration, reducing the
scan to O(number_of_namespaces).
--
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]
