Skip to content

fix: manual compact unexpected start #1565

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

fix: manual compact unexpected start #1565

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
e524ab6
fix_manual_compact_unexpected_start
Jul 6, 2023
46d8f21
Merge branch 'apache:master' into fix_manual_compact_unexpected_start
ninsmiracle Jul 7, 2023
24fb407
add unit test case
Jul 7, 2023
75ab897
add _time_natural_increase
Jul 7, 2023
e607a78
add unit test
Jul 10, 2023
1371b6c
del useless code
Jul 11, 2023
48c3469
reformat code
Jul 11, 2023
6e1b9ac
del useless note
Jul 11, 2023
c4e4ff5
add some comment for new manual compact state
Aug 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions src/server/pegasus_manual_compact_service.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,9 @@ pegasus_manual_compact_service::pegasus_manual_compact_service(pegasus_server_im
: replica_base(*app),
_app(app),
_disabled(false),
_time_natural_increase(false),
_max_concurrent_running_count(INT_MAX),
_manual_compact_first_day_s(0),
_manual_compact_enqueue_time_ms(0),
_manual_compact_start_running_time_ms(0),
_manual_compact_last_finish_time_ms(0),
Expand Down Expand Up @@ -181,6 +183,9 @@ bool pegasus_manual_compact_service::check_periodic_compact(
{
auto find = envs.find(MANUAL_COMPACT_PERIODIC_TRIGGER_TIME_KEY);
if (find == envs.end()) {
// if user remove MANUAL_COMPACT_PERIODIC_TRIGGER_TIME_KEY for this app
_manual_compact_first_day_s.store(0);
_time_natural_increase.store(false);
return false;
}

Expand All @@ -191,6 +196,11 @@ bool pegasus_manual_compact_service::check_periodic_compact(
return false;
}

if (0 == _manual_compact_first_day_s.load()) {
int64_t first_day_midnight = dsn::utils::get_unix_sec_today_midnight();
_manual_compact_first_day_s.store(first_day_midnight);
}

std::set<int64_t> trigger_time;
for (auto &tts : trigger_time_strs) {
int64_t tt = dsn::utils::hh_mm_today_to_unix_sec(tts);
Expand All @@ -204,8 +214,16 @@ bool pegasus_manual_compact_service::check_periodic_compact(
}

auto now = static_cast<int64_t>(now_timestamp());
int64_t cur_day_midnight = dsn::utils::get_unix_sec_today_midnight();
for (auto t : trigger_time) {
auto t_ms = t * 1000;

if (_manual_compact_first_day_s.load() == cur_day_midnight && t_ms < now &&
Copy link
Member

Choose a reason for hiding this comment

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

It's allowed to set multiple periodic compact tasks on a table, for example there are 2 tasks set to be executed at 02:00 and 16:00, now real world time is 15:00, will the second task (16:00 one) to be executed normally?

!_time_natural_increase.load()) {
return false;
}

_time_natural_increase.store(true);
if (_manual_compact_last_finish_time_ms.load() < t_ms && t_ms < now) {
return true;
}
Expand Down
11 changes: 11 additions & 0 deletions src/server/pegasus_manual_compact_service.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,18 @@ class pegasus_manual_compact_service : public dsn::replication::replica_base

// manual compact state
std::atomic<bool> _disabled;
// when trigger time is larger than current time,there are two situation
// 1. The current day is the day when the user first sets the manual compaction trigger time,
// and the user sets a time earlier than the time of this action.
// 2. The current day is not the day when the user first sets the time. We should ensure that
// manual compaction is executed.
// The flag _time_natural_increase indicates if the current time is later than the trigger time,
// causing time to progress.
// The flag _manual_compact_first_day_s records the day's midnight when the user set the manual
// compaction time.
std::atomic<bool> _time_natural_increase;
std::atomic<int> _max_concurrent_running_count;
std::atomic<uint64_t> _manual_compact_first_day_s;
Copy link
Member

@acelyc111 acelyc111 Aug 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Could you please add some comments for the newly added variable?

The logic is too complex if adding these variables, if the current "periodic", "max_concurrent_running" features are not well implemented, I don't mind you to rewrite or even remove them.

std::atomic<uint64_t> _manual_compact_enqueue_time_ms;
std::atomic<uint64_t> _manual_compact_start_running_time_ms;
std::atomic<uint64_t> _manual_compact_last_finish_time_ms;
Expand Down
57 changes: 57 additions & 0 deletions src/server/test/manual_compact_service_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,11 +59,21 @@ class manual_compact_service_test : public pegasus_server_test_base
static_cast<uint64_t>(ts * 1000));
}

void set_first_run_time(int64_t ts)
{
manual_compact_svc->_manual_compact_first_day_s.store(static_cast<uint64_t>(ts));
}

void set_mock_now(uint64_t mock_now_sec)
{
manual_compact_svc->_mock_now_timestamp = mock_now_sec * 1000;
}

void set_time_natural_increase(bool ok)
{
manual_compact_svc->_time_natural_increase.store(ok);
}

void check_compact_disabled(const std::map<std::string, std::string> &envs, bool ok)
{
ASSERT_EQ(ok, manual_compact_svc->check_compact_disabled(envs))
Expand Down Expand Up @@ -324,5 +334,52 @@ TEST_F(manual_compact_service_test, check_manual_compact_state_1h_interval)
check_manual_compact_state(false, "3611s past, start not ok");
}

TEST_F(manual_compact_service_test, check_periodic_compact_first_time)
{
std::map<std::string, std::string> envs;

// user first set MANUAL_COMPACT_PERIODIC_TRIGGER_TIME_KEY
envs[MANUAL_COMPACT_PERIODIC_TRIGGER_TIME_KEY] = "02:00";
set_mock_now((uint64_t)dsn::utils::hh_mm_today_to_unix_sec("10:00"));
set_first_run_time(dsn::utils::get_unix_sec_today_midnight());
set_time_natural_increase(false);
check_periodic_compact(envs, false);

// now time to next day 00:00:01
set_mock_now((uint64_t)dsn::utils::hh_mm_today_to_unix_sec("23:59") + 61);
check_periodic_compact(envs, false);

// now time to next day 02:00:01
set_mock_now((uint64_t)dsn::utils::hh_mm_today_to_unix_sec("23:59") + 7261);
set_time_natural_increase(true);
check_periodic_compact(envs, true);

// suppose compacted at today's 02:30
set_compact_time(dsn::utils::hh_mm_today_to_unix_sec("02:30"));

// change value,but not first time set KEY
envs[MANUAL_COMPACT_PERIODIC_TRIGGER_TIME_KEY] = "10:00";
set_mock_now((uint64_t)dsn::utils::hh_mm_today_to_unix_sec("14:00"));
check_periodic_compact(envs, true);

// user remove MANUAL_COMPACT_PERIODIC_TRIGGER_TIME_KEY for this app
envs.erase(MANUAL_COMPACT_PERIODIC_TRIGGER_TIME_KEY);
set_first_run_time(0);
set_time_natural_increase(false);
set_mock_now((uint64_t)dsn::utils::hh_mm_today_to_unix_sec("14:00"));
check_periodic_compact(envs, false);

// user first set MANUAL_COMPACT_PERIODIC_TRIGGER_TIME_KEY
envs[MANUAL_COMPACT_PERIODIC_TRIGGER_TIME_KEY] = "15:00";
set_mock_now((uint64_t)dsn::utils::hh_mm_today_to_unix_sec("14:00"));
set_first_run_time(dsn::utils::get_unix_sec_today_midnight());
check_periodic_compact(envs, false);

// now time to 15:01(time natural increase)
set_first_run_time(true);
set_mock_now((uint64_t)dsn::utils::hh_mm_today_to_unix_sec("15:01"));
check_periodic_compact(envs, true);
}

} // namespace server
} // namespace pegasus