This is an automated email from the ASF dual-hosted git repository.
wangdan pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-pegasus.git
The following commit(s) were added to refs/heads/master by this push:
new 7d581a45f fix(backup): limit the backup interval to be larger than
86400 seconds (#1883)
7d581a45f is described below
commit 7d581a45f8744b8e6b5388aa496c1ce337284ee5
Author: Yingchun Lai <[email protected]>
AuthorDate: Thu Feb 1 21:06:40 2024 +0800
fix(backup): limit the backup interval to be larger than 86400 seconds
(#1883)
---
src/meta/meta_backup_service.cpp | 39 +++++++++++++++++++------
src/meta/test/backup_test.cpp | 2 +-
src/test/function_test/restore/test_restore.cpp | 8 +----
3 files changed, 32 insertions(+), 17 deletions(-)
diff --git a/src/meta/meta_backup_service.cpp b/src/meta/meta_backup_service.cpp
index 2ed5425c5..93c4ac739 100644
--- a/src/meta/meta_backup_service.cpp
+++ b/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,7 +1647,8 @@ 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,
@@ -1636,9 +1656,10 @@ void
backup_service::modify_backup_policy(configuration_modify_backup_policy_rpc
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);
}
}
diff --git a/src/meta/test/backup_test.cpp b/src/meta/test/backup_test.cpp
index b27ba4995..7d2500bf8 100644
--- a/src/meta/test/backup_test.cpp
+++ b/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);
diff --git a/src/test/function_test/restore/test_restore.cpp
b/src/test/function_test/restore/test_restore.cpp
index e99ef62ea..107051dfd 100644
--- a/src/test/function_test/restore/test_restore.cpp
+++ b/src/test/function_test/restore/test_restore.cpp
@@ -61,15 +61,9 @@ public:
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)); }
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
