Skip to content
This repository has been archived by the owner on Jun 23, 2022. It is now read-only.

BREAKING CHANGE: fix behavior when backup period less than 1day #724

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

zhangyifan27
Copy link
Contributor

@zhangyifan27 zhangyifan27 commented Jan 11, 2021

Before this patch, we cannot start periodic backup correctly when backup_interval_secends less than one_day_seconds, because there is some issues in should_start_backup_unlocked().

Now we start the first backup only when cur_time == start_time, then periodically start backup at intervals defined by backup_interval_secends.
The start time of subsequent backups is only related to last_backup_start_time and backup_interval_secends, so the start_time of a policy shouldn't be modified if a backup has already started.

src/meta/meta_backup_service.cpp Outdated Show resolved Hide resolved
@@ -1540,15 +1515,28 @@ void backup_service::modify_backup_policy(configuration_modify_backup_policy_rpc
}

if (request.__isset.start_time) {
if (context_ptr->has_backup_history() || context_ptr->is_under_backuping()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding context_ptr->has_backup_history() condition, it means if the policy has already generated the backup, its start time can not be modified. Could you please explain why it should not be modified to me?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because start_time only indicates the first time to start backup, if we have already started a backup, we shouldn't modify it.

Copy link
Contributor

@hycdong hycdong Jan 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For once backup, start_time indicates the first time to start backup, for periodical backup, I think it should be allowed to be updated, it indicates this round time to start backup, it is controlled by last start time and backup interval. Making start_time allowed to be updated, it will make function flexible, I recommend remain it if it won't make code too hard to understand.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The start time of subsequent backups is only related to last_backup_start_time and backup_interval_secends, if users want to modify next backup time, they could modify backup_interval_secends.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you almost persuade me. Let me take an example to explain, there were a policy whose start time is 1:00, interval time is 1 day, if we would like update it to backup everyday 2:00. In this case, we wouldn't update interval time, if we don't provide updating start_time, we can only create another policy or update interval twice.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let‘s imagine how would user use this feature, maybe it's almost like how would you set you clock, there is no such a configurable "interval" concept, the interval should always be fixed value like day(or week, month, etc).
If user want to backup multiple times in one day, she should set multiple policies. On the other hand, I believe backup multiple times a day isn't a regular requirement, we don't need to make our design and code too complex to satisfy it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the design and code is complex. start_time is the first time to start a backup of a policy, backup_interval_seconds is interval between two backups, which is quite easy to understand. If users want to modify start_time, they should create another policy.

If we all agree that we should keep backup_interval_seconds, then any interval could be set, we shouldn't only support the fixed interval(day/week/month).

struct backup_start_time
{
int32_t hour; // [0 ~24)
int32_t minute; // [0 ~ 60)
int32_t hour;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to change the type of hour and minute to uint8_t

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dsn::utils::time_ms_to_date_time use int32_t for hour, min, sec, so we also use int32_t here.
Maybe we should consider use uint8_t to replace int32_t in dsn::utils::time_ms_to_date_time, if not , even if we changed interger type here, type conversions would still be needed.

@neverchanje neverchanje changed the title fix(backup): fix should_start_backup_unlocked() fix(backup): fix behavior when backup period less than 1day Jan 15, 2021
src/meta/meta_backup_service.cpp Outdated Show resolved Hide resolved
src/meta/meta_backup_service.cpp Show resolved Hide resolved
Comment on lines +107 to +108
if (hour == 24 && minute == 0) {
// only for tests
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is too tricky. I don't want any test code appears in such way.

Try this: https://github.com/XiaoMi/rdsn/blob/master/include/dsn/utility/fail_point.h

FAIL_POINT_INJECT_F(should_start_backup, []() -> bool {
  return true;
});

When you in test calling fail::cfg("should_start_backup", return()), and this func will return true.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually we didn't call should_start_backup directly in tests, we called issue_new_backup_unlocked and expected starting a new backup in policy_context_test.test_app_dropped_during_backup.

@neverchanje neverchanje changed the title fix(backup): fix behavior when backup period less than 1day BREAKING CHANGE: fix behavior when backup period less than 1day Jan 20, 2021
src/meta/meta_backup_service.cpp Outdated Show resolved Hide resolved
@@ -1540,15 +1515,28 @@ void backup_service::modify_backup_policy(configuration_modify_backup_policy_rpc
}

if (request.__isset.start_time) {
if (context_ptr->has_backup_history() || context_ptr->is_under_backuping()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let‘s imagine how would user use this feature, maybe it's almost like how would you set you clock, there is no such a configurable "interval" concept, the interval should always be fixed value like day(or week, month, etc).
If user want to backup multiple times in one day, she should set multiple policies. On the other hand, I believe backup multiple times a day isn't a regular requirement, we don't need to make our design and code too complex to satisfy it.

Co-authored-by: Yingchun Lai <405403881@qq.com>
@neverchanje
Copy link
Contributor

neverchanje commented Feb 20, 2021

@acelyc111 The new implementation is actually making the kernel design simpler than before, considering the deleted code that handles clock drift. We don't have to avoid the two backups being too close, or similiar edge cases.

From the users perspective, this design is primitive and prone to misuse. I prefer this problem to be solved in Backup Manager. You can set up rules or limitations in Backup Manager.

@zhangyifan27 zhangyifan27 marked this pull request as draft March 18, 2021 09:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants