fix(backup): limit the backup interval to be larger than 86400 seconds (apache#1883)

Author: acelyc111

src/meta/meta_backup_service.cpp

@@ -75,6 +75,28 @@ metric_entity_ptr instantiate_backup_policy_metric_entity(const std::string &pol
     return METRIC_ENTITY_backup_policy.instantiate(entity_id, {{"policy_name", policy_name}});
 }
 
+bool validate_backup_interval(int64_t backup_interval_seconds, std::string &hint_message)
+{
+    // The backup interval must be larger than checkpoint reserve time.
+    // Or the next cold backup checkpoint may be cleared by the clear operation.
+    if (backup_interval_seconds <= FLAGS_cold_backup_checkpoint_reserve_minutes * 60) {
+        hint_message = fmt::format(
+            "backup interval must be larger than cold_backup_checkpoint_reserve_minutes={}",
+            FLAGS_cold_backup_checkpoint_reserve_minutes);
+        return false;
+    }
+
+    // There is a bug occurred in backup if the backup interval is less than 1 day, this is a
+    // temporary resolution, the long term plan is to remove periodic backup.
+    // See details https://github.com/apache/incubator-pegasus/issues/1081.
+    if (backup_interval_seconds < 86400) {
+        hint_message = fmt::format("backup interval must be >= 86400 (1 day)");
+        return false;
+    }
+
+    return true;
+}
+
 } // anonymous namespace
 
 backup_policy_metrics::backup_policy_metrics(const std::string &policy_name)
@@ -1281,13 +1303,10 @@ void backup_service::add_backup_policy(dsn::message_ex *msg)
     std::set<int32_t> app_ids;
     std::map<int32_t, std::string> app_names;
 
-    // The backup interval must be greater than checkpoint reserve time.
-    // Or the next cold backup checkpoint may be cleared by the clear operation.
-    if (request.backup_interval_seconds <= FLAGS_cold_backup_checkpoint_reserve_minutes * 60) {
+    std::string hint_message;
+    if (!validate_backup_interval(request.backup_interval_seconds, hint_message)) {
         response.err = ERR_INVALID_PARAMETERS;
-        response.hint_message = fmt::format(
-            "backup interval must be greater than FLAGS_cold_backup_checkpoint_reserve_minutes={}",
-            FLAGS_cold_backup_checkpoint_reserve_minutes);
+        response.hint_message = hint_message;
         _meta_svc->reply_data(msg, response);
         msg->release_ref();
         return;
@@ -1628,17 +1647,19 @@ void backup_service::modify_backup_policy(configuration_modify_backup_policy_rpc
     }
 
     if (request.__isset.new_backup_interval_sec) {
-        if (request.new_backup_interval_sec > 0) {
+        std::string hint_message;
+        if (validate_backup_interval(request.new_backup_interval_sec, hint_message)) {
             LOG_INFO("{}: policy will change backup interval from {}s to {}s",
                      cur_policy.policy_name,
                      cur_policy.backup_interval_seconds,
                      request.new_backup_interval_sec);
             cur_policy.backup_interval_seconds = request.new_backup_interval_sec;
             have_modify_policy = true;
         } else {
-            LOG_WARNING("{}: invalid backup_interval_sec({})",
+            LOG_WARNING("{}: invalid backup_interval_sec({}), {}",
                         cur_policy.policy_name,
-                        request.new_backup_interval_sec);
+                        request.new_backup_interval_sec,
+                        hint_message);
         }
     }
 

src/meta/test/backup_test.cpp

@@ -821,7 +821,7 @@ TEST_F(meta_backup_service_test, test_add_backup_policy)
         fake_wait_rpc(r, resp);
 
         std::string hint_message = fmt::format(
-            "backup interval must be greater than FLAGS_cold_backup_checkpoint_reserve_minutes={}",
+            "backup interval must be larger than cold_backup_checkpoint_reserve_minutes={}",
             FLAGS_cold_backup_checkpoint_reserve_minutes);
         ASSERT_EQ(ERR_INVALID_PARAMETERS, resp.err);
         ASSERT_EQ(hint_message, resp.hint_message);

src/test/function_test/restore/test_restore.cpp

@@ -61,15 +61,9 @@ class restore_test : public test_util
         NO_FATALS(write_data(kTestCount));
 
         std::vector<int32_t> app_ids({table_id_});
-        // NOTICE: we enqueue a time task to check whether policy should start backup periodically,
-        // the period is 5min, so the time between two backup is at least 5min, but if we set the
-        // 'backup_interval_seconds' smaller enough such as smaller than the time of finishing once
-        // backup, we can start next backup immediately when current backup is finished.
-        // The backup interval must be greater than checkpoint reserve time, see
-        // backup_service::add_backup_policy() for details.
         ASSERT_EQ(
             ERR_OK,
-            ddl_client_->add_backup_policy("policy_1", "local_service", app_ids, 700, 6, "24:0"));
+            ddl_client_->add_backup_policy("policy_1", "local_service", app_ids, 86400, 6, "24:0"));
     }
 
     void TearDown() override { ASSERT_EQ(ERR_OK, ddl_client_->drop_app(table_name_, 0)); }