chore: help users to fix a mistake of setting quotes in the flagfile (#…

…2092) * chore: help users to fix a common mistake of setting quotes in the flagfile Specifically, the confusion is often around the cron expression. --------- Signed-off-by: Roman Gershman <[email protected]>
dragonflydb · Oct 30, 2023 · 8a65aec · 8a65aec
1 parent 39c1827
commit 8a65aec
Show file tree

Hide file tree

Showing 4 changed files with 28 additions and 6 deletions.
diff --git a/src/server/detail/save_stages_controller.cc b/src/server/detail/save_stages_controller.cc
@@ -66,7 +66,17 @@ void ExtendDfsFilenameWithShard(int shard, string_view extension, fs::path* file
 }  // namespace
 
 GenericError ValidateFilename(const fs::path& filename, bool new_version) {
-  bool is_cloud_path = IsCloudPath(filename.string());
+  if (filename.empty()) {
+    return {};
+  }
+
+  string filename_str = filename.string();
+  if (filename_str.front() == '"') {
+    return {
+        "filename should not start with '\"', could it be that you put quotes in the flagfile?"};
+  }
+
+  bool is_cloud_path = IsCloudPath(filename_str);
 
   if (!filename.parent_path().empty() && !is_cloud_path) {
     return {absl::StrCat("filename may not contain directory separators (Got \"", filename.c_str(),
@@ -319,6 +329,9 @@ GenericError SaveStagesController::BuildFullPath() {
   }
 
   fs::path filename = basename_.empty() ? GetFlag(FLAGS_dbfilename) : basename_;
+  if (filename.empty())
+    return {"filename is not specified"};
+
   if (auto err = ValidateFilename(filename, use_dfs_format_); err)
     return err;
 

diff --git a/src/server/detail/snapshot_storage.cc b/src/server/detail/snapshot_storage.cc
@@ -96,7 +96,7 @@ io::ReadonlyFileOrError FileSnapshotStorage::OpenReadFile(const std::string& pat
 io::Result<std::string, GenericError> FileSnapshotStorage::LoadPath(std::string_view dir,
                                                                     std::string_view dbfilename) {
   if (dbfilename.empty())
-    return "";
+    return {};
 
   fs::path data_folder;
   if (dir.empty()) {

diff --git a/src/server/server_family.cc b/src/server/server_family.cc
@@ -421,14 +421,20 @@ std::optional<cron::cronexpr> InferSnapshotCronExpr() {
       LOG(WARNING) << "Invalid snapshot time specifier " << save_time;
     }
   } else if (!snapshot_cron_exp.empty()) {
+    if (absl::StartsWith(snapshot_cron_exp, "\"")) {
+      LOG(WARNING) << "Invalid snapshot cron expression `" << snapshot_cron_exp
+                   << "`, could it be that you put quotes in the flagfile?";
+      return nullopt;
+    }
     raw_cron_expr = "0 " + snapshot_cron_exp;
   }
 
   if (!raw_cron_expr.empty()) {
     try {
+      VLOG(1) << "creating cron from: `" << raw_cron_expr << "`";
       return std::optional<cron::cronexpr>(cron::make_cron(raw_cron_expr));
     } catch (const cron::bad_cronexpr& ex) {
-      LOG(WARNING) << "Invalid cron expression: " << ex.what();
+      LOG(WARNING) << "Invalid cron expression: " << raw_cron_expr;
     }
   }
   return std::nullopt;
@@ -1291,6 +1297,9 @@ void ServerFamily::Memory(CmdArgList args, ConnectionContext* cntx) {
   return mem_cmd.Run(args);
 }
 
+// SAVE [DF|RDB] [basename]
+// Allows saving the snapshot of the dataset on disk, potentially overriding the format
+// and the snapshot name.
 void ServerFamily::Save(CmdArgList args, ConnectionContext* cntx) {
   string err_detail;
   bool new_version = absl::GetFlag(FLAGS_df_snapshot_format);

diff --git a/tests/dragonfly/replication_test.py b/tests/dragonfly/replication_test.py
@@ -1551,13 +1551,13 @@ async def test_df_crash_on_replicaof_flag(df_local_factory):
     replica.start()
 
     c_master = aioredis.Redis(port=master.port)
-    c_replica = aioredis.Redis(port=replica.port)
+    c_replica = aioredis.Redis(port=replica.port, decode_responses=True)
 
     await wait_available_async(c_master)
     await wait_available_async(c_replica)
 
-    res = await c_replica.execute_command("BGSAVE")
-    assert True == res
+    res = await c_replica.execute_command("SAVE DF myfile")
+    assert "OK" == res
 
     res = await c_replica.execute_command("DBSIZE")
     assert res == 0