chore: clean up of deprecated flags (#4545)

Also fix sentinel test by using a precise redis-server version.
Finally, add pytest warnings filter to reduce noise

Signed-off-by: Roman Gershman <roman@dragonflydb.io>

src/core/sorted_map.cc

@@ -21,8 +21,6 @@ extern "C" {
 
 using namespace std;
 
-ABSL_RETIRED_FLAG(bool, use_zset_tree, true, "If true use b+tree for zset implementation");
-
 namespace dfly {
 namespace detail {
 

src/server/container_utils.cc

@@ -22,8 +22,6 @@ extern "C" {
 #include "redis/zset.h"
 }
 
-ABSL_FLAG(bool, singlehop_blocking, true, "Use single hop optimization for blocking commands");
-
 namespace dfly::container_utils {
 using namespace std;
 namespace {
@@ -336,7 +334,7 @@ OpResult<string> RunCbOnFirstNonEmptyBlocking(Transaction* trans, int req_obj_ty
   // If we don't find anything, we abort concluding and keep scheduled.
   // Slow path: schedule, find results from shards, execute action if found.
   OpResult<ShardFFResult> result;
-  if (trans->GetUniqueShardCnt() == 1 && absl::GetFlag(FLAGS_singlehop_blocking)) {
+  if (trans->GetUniqueShardCnt() == 1) {
     auto res = FindFirstNonEmptySingleShard(trans, req_obj_type, func);
     if (res.ok()) {
       if (info)

src/server/generic_family_test.cc

@@ -24,8 +24,6 @@ using namespace util;
 using namespace boost;
 using absl::StrCat;
 
-ABSL_DECLARE_FLAG(bool, list_rdb_encode_v2);
-
 namespace dfly {
 
 class GenericFamilyTest : public BaseFamilyTest {};
@@ -565,17 +563,16 @@ TEST_F(GenericFamilyTest, Persist) {
 }
 
 TEST_F(GenericFamilyTest, Dump) {
-  ASSERT_THAT(RDB_SER_VERSION, 9);
-  absl::SetFlag(&FLAGS_list_rdb_encode_v2, false);
+  ASSERT_EQ(RDB_SER_VERSION, 9);
   uint8_t EXPECTED_STRING_DUMP[13] = {0x00, 0xc0, 0x13, 0x09, 0x00, 0x23, 0x13,
                                       0x6f, 0x4d, 0x68, 0xf6, 0x35, 0x6e};
   uint8_t EXPECTED_HASH_DUMP[] = {0x0d, 0x12, 0x12, 0x00, 0x00, 0x00, 0x0d, 0x00, 0x00, 0x00,
                                   0x02, 0x00, 0x00, 0xfe, 0x13, 0x03, 0xc0, 0xd2, 0x04, 0xff,
                                   0x09, 0x00, 0xb1, 0x0b, 0xae, 0x6c, 0x23, 0x5d, 0x17, 0xaa};
 
-  uint8_t EXPECTED_LIST_DUMP[] = {0x0e, 0x01, 0x0e, 0x0e, 0x00, 0x00, 0x00, 0x0a, 0x00,
-                                  0x00, 0x00, 0x01, 0x00, 0x00, 0xfe, 0x14, 0xff, 0x09,
-                                  0x00, 0xba, 0x1e, 0xa9, 0x6b, 0xba, 0xfe, 0x2d, 0x3f};
+  uint8_t EXPECTED_LIST_DUMP[] = {0x12, 0x01, 0x02, '\t', '\t', 0x00, 0x00, 0x00,
+                                  0x01, 0x00, 0x14, 0x01, 0xff, '\t', 0x00, 0xfb,
+                                  0xbd, 0x36, 0xf8, 0xb4, 't',  '%',  ';'};
 
   // Check string dump
   auto resp = Run({"set", "z", "19"});
@@ -588,7 +585,7 @@ TEST_F(GenericFamilyTest, Dump) {
   EXPECT_EQ(1, CheckedInt({"rpush", "l", "20"}));
   resp = Run({"dump", "l"});
   dump = resp.GetBuf();
-  CHECK_EQ(ToSV(dump), ToSV(EXPECTED_LIST_DUMP));
+  CHECK_EQ(ToSV(dump), ToSV(EXPECTED_LIST_DUMP)) << absl::CHexEscape(resp.GetString());
 
   // Check for hash dump
   EXPECT_EQ(1, CheckedInt({"hset", "z2", "19", "1234"}));
@@ -873,7 +870,7 @@ TEST_F(GenericFamilyTest, RestoreOOM) {
 }
 
 TEST_F(GenericFamilyTest, Bug4466) {
-  auto resp = Run({"SCAN","9223372036854775808"});  // an invalid cursor should not crash us.
+  auto resp = Run({"SCAN", "9223372036854775808"});  // an invalid cursor should not crash us.
   EXPECT_THAT(resp, RespElementsAre("0", RespElementsAre()));
 }
 

src/server/rdb_save.cc

@@ -49,12 +49,12 @@ ABSL_FLAG(dfly::CompressionMode, compression_mode, dfly::CompressionMode::MULTI_
           "set 2 for multi entry zstd compression on df snapshot and single entry on rdb snapshot,"
           "set 3 for multi entry lz4 compression on df snapshot and single entry on rdb snapshot");
 
-// TODO: to retire both flags in v1.27 (Jan 2025)
-ABSL_FLAG(bool, list_rdb_encode_v2, true,
+ABSL_RETIRED_FLAG(bool, list_rdb_encode_v2, true,
           "V2 rdb encoding of list uses listpack encoding format, compatible with redis 7. V1 rdb "
           "enconding of list uses ziplist encoding compatible with redis 6");
 
-ABSL_FLAG(bool, stream_rdb_encode_v2, false,
+// TODO: to retire this flag in v1.31
+ABSL_FLAG(bool, stream_rdb_encode_v2, true,
           "V2 uses format, compatible with redis 7.2 and Dragonfly v1.26+, while v1 format "
           "is compatible with redis 6");
 
@@ -173,8 +173,7 @@ uint8_t RdbObjectType(const PrimeValue& pv) {
       return RDB_TYPE_STRING;
     case OBJ_LIST:
       if (compact_enc == OBJ_ENCODING_QUICKLIST || compact_enc == kEncodingQL2) {
-        return absl::GetFlag(FLAGS_list_rdb_encode_v2) ? RDB_TYPE_LIST_QUICKLIST_2
-                                                       : RDB_TYPE_LIST_QUICKLIST;
+        return RDB_TYPE_LIST_QUICKLIST_2;
       }
       break;
     case OBJ_SET:
@@ -375,49 +374,19 @@ error_code RdbSerializer::SaveListObject(const PrimeValue& pv) {
     DVLOG(3) << "QL node (encoding/container/sz): " << node->encoding << "/" << node->container
              << "/" << node->sz;
 
-    if (absl::GetFlag(FLAGS_list_rdb_encode_v2)) {
-      // Use listpack encoding
-      SaveLen(node->container);
-      if (quicklistNodeIsCompressed(node)) {
-        void* data;
-        size_t compress_len = quicklistGetLzf(node, &data);
+    // Use listpack encoding
+    SaveLen(node->container);
+    if (quicklistNodeIsCompressed(node)) {
+      void* data;
+      size_t compress_len = quicklistGetLzf(node, &data);
 
-        RETURN_ON_ERR(SaveLzfBlob(Bytes{reinterpret_cast<uint8_t*>(data), compress_len}, node->sz));
-      } else {
-        RETURN_ON_ERR(SaveString(node->entry, node->sz));
-        FlushState flush_state = FlushState::kFlushMidEntry;
-        if (node->next == nullptr)
-          flush_state = FlushState::kFlushEndEntry;
-        FlushIfNeeded(flush_state);
-      }
+      RETURN_ON_ERR(SaveLzfBlob(Bytes{reinterpret_cast<uint8_t*>(data), compress_len}, node->sz));
     } else {
-      // Use ziplist encoding
-      if (QL_NODE_IS_PLAIN(node)) {
-        RETURN_ON_ERR(SavePlainNodeAsZiplist(node));
-      } else {
-        // listpack node
-        uint8_t* lp = node->entry;
-        uint8_t* decompressed = NULL;
-
-        if (quicklistNodeIsCompressed(node)) {
-          void* data;
-          size_t compress_len = quicklistGetLzf(node, &data);
-          decompressed = (uint8_t*)zmalloc(node->sz);
-
-          if (lzf_decompress(data, compress_len, decompressed, node->sz) == 0) {
-            /* Someone requested decompress, but we can't decompress.  Not good. */
-            zfree(decompressed);
-            return make_error_code(errc::illegal_byte_sequence);
-          }
-          lp = decompressed;
-        }
-
-        auto cleanup = absl::MakeCleanup([=] {
-          if (decompressed)
-            zfree(decompressed);
-        });
-        RETURN_ON_ERR(SaveListPackAsZiplist(lp));
-      }
+      RETURN_ON_ERR(SaveString(node->entry, node->sz));
+      FlushState flush_state = FlushState::kFlushMidEntry;
+      if (node->next == nullptr)
+        flush_state = FlushState::kFlushEndEntry;
+      FlushIfNeeded(flush_state);
     }
     node = node->next;
   }

src/server/replica.cc

@@ -49,10 +49,6 @@ ABSL_FLAG(
     int, replica_priority, 100,
     "Published by info command for sentinel to pick replica based on score during a failover");
 
-// TODO: Remove this flag on release >= 1.22
-ABSL_FLAG(bool, replica_reconnect_on_master_restart, false,
-          "Deprecated - please use --break_replication_on_master_restart.");
-
 namespace dfly {
 
 using namespace std;
@@ -339,8 +335,7 @@ std::error_code Replica::HandleCapaDflyResp() {
   // If we're syncing a different replication ID, drop the saved LSNs.
   string_view master_repl_id = ToSV(LastResponseArgs()[0].GetBuf());
   if (master_context_.master_repl_id != master_repl_id) {
-    if ((absl::GetFlag(FLAGS_replica_reconnect_on_master_restart) ||
-         absl::GetFlag(FLAGS_break_replication_on_master_restart)) &&
+    if (absl::GetFlag(FLAGS_break_replication_on_master_restart) &&
         !master_context_.master_repl_id.empty()) {
       LOG(ERROR) << "Encountered different master repl id (" << master_repl_id << " vs "
                  << master_context_.master_repl_id << ")";

src/server/server_family.cc

@@ -870,7 +870,6 @@ void ServerFamily::Init(util::AcceptServer* acceptor, std::vector<facade::Listen
   config_registry.RegisterMutable("tls_ca_cert_dir");
   config_registry.RegisterMutable("replica_priority");
   config_registry.RegisterMutable("lua_undeclared_keys_shas");
-  config_registry.RegisterMutable("list_rdb_encode_v2");
 
   pb_task_ = shard_set->pool()->GetNextProactor();
   if (pb_task_->GetKind() == ProactorBase::EPOLL) {

tests/dragonfly/instance.py

@@ -480,8 +480,10 @@ class RedisServer:
         self.port = port
         self.proc = None
 
-    def start(self, **kwargs):
-        servers = ["redis-server-6.2.11", "redis-server-7.2.2", "valkey-server-8.0.1"]
+    def start(self, redis7=None, **kwargs):
+        servers = ["redis-server-7.2.2"]
+        if not redis7:
+            servers += ["redis-server-6.2.11", "valkey-server-8.0.1"]
         command = [
             random.choice(servers),
             f"--port {self.port}",

tests/dragonfly/sentinel_test.py

@@ -81,7 +81,7 @@ class Sentinel:
         logging.info(self.config_file.read_text())
 
         self.proc = subprocess.Popen(
-            ["redis-server", f"{self.config_file.absolute()}", "--sentinel"]
+            ["redis-server-6.2.11", f"{self.config_file.absolute()}", "--sentinel"]
         )
 
     def stop(self):

tests/dragonfly/snapshot_test.py

@@ -155,7 +155,6 @@ async def test_dbfilenames(
         **BASIC_ARGS,
         "proactor_threads": 4,
         "dbfilename": "test-redis-load-rdb",        
-        "list_rdb_encode_v2": "false",  # Needed for compatibility with Redis 6
     }
 )
 async def test_redis_load_snapshot(
@@ -176,7 +175,7 @@ async def test_redis_load_snapshot(
     await async_client.connection_pool.disconnect()
     df_server.stop()
 
-    redis_local_server.start(dir=tmp_dir, dbfilename="test-redis-load-rdb.rdb")
+    redis_local_server.start(dir=tmp_dir, redis7=True, dbfilename="test-redis-load-rdb.rdb")
     await asyncio.sleep(1)
     c_master = aioredis.Redis(port=redis_local_server.port)
     await c_master.ping()

tests/pytest.ini

@@ -10,3 +10,5 @@ markers =
   slow: marks tests as slow (deselect with '-m "not slow"')
   opt_only: marks tests that are only reasonable to run against an opt-built Dragonfly
   exclude_epoll: marks tests that should not run on epoll socket
+filterwarnings =
+    ignore::DeprecationWarning