Skip to content
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.

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

feat: raise load_from_private_log priority from LOW to COMMON #2184

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

Conversation

ninsmiracle
Copy link
Contributor

@ninsmiracle ninsmiracle commented Jan 20, 2025

What problem does this PR solve?

#2183

What is changed and how does it work?

Raise the priority of load_from_private_log from LOW to COMMON.

Performance Testing

I do some test cases as following:
In these test cases, I first wrote 8k QPS of write traffic to the master cluster (this is the traffic that my test cluster will not generate dup log backlogs) to verify the effect of the priority modification. Then I wrote 20k QPS of write traffic to the master cluster (this is the traffic that my test cluster will generate a certain degree of dup log backlogs) to verify the effect of the priority modification.

load/ship task priority load_from_private_log task priority qps duplicate_log_batch_bytes plog Maximum backlog Master cluster write delay p99 master/slave dup delay
LOW LOW 8K 4096 9k 1ms p95 127ms、p99 27473ms
LOW COMMON 8K 4096 200 1ms p95 101ms、p99 109ms
HIGH HIGH 8K 4096 150 1ms p95 107ms、p99 115ms
LOW LOW 20K 4096 61K 1.5ms p95 139ms、p99 20506ms
LOW COMMON 20K 4096 42K 1.5ms p95 126ms、p99 18127ms
LOW COMMON 20K 0 Continue to increase over time 1.5ms 95 10618ms、p99 303519ms

As you see , change the priority from LOW to COMMON of load_from_private_log will not t increase the online delay. And priority from LOW to HIGH is no benefit for further speeding up duplication.

So based on the above experimental conclusions, I think this issues' argument is valid.

@github-actions github-actions bot added the cpp label Jan 20, 2025
@acelyc111 acelyc111 closed this Jan 20, 2025
@acelyc111 acelyc111 reopened this Jan 20, 2025
acelyc111
acelyc111 previously approved these changes Feb 12, 2025
namespace dsn {
namespace replication {
DSN_DEFINE_string(replication,
load_from_private_log_level,
Copy link
Contributor

Choose a reason for hiding this comment

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

load_from_private_log_level seems to a configuration for the log level of something. Also, it does not show the relationship to duplication. In addition, this configuration is essentially a task code specifying a class of tasks running on the specific thread pool. We could use dup_load_plog_task to show a configurable task for incremental loading from private logs while duplicating.

Suggested change
load_from_private_log_level,
dup_load_plog_task,

"The level of load_from_private_log when doing a duplication.Be false means the "
"task level of replaing plog is low, otherwise the task level is common (We do "
"not recommend high level)");
DSN_TAG_VARIABLE(load_from_private_log_level, FT_MUTABLE);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
DSN_TAG_VARIABLE(load_from_private_log_level, FT_MUTABLE);
DSN_TAG_VARIABLE(dup_load_plog_task, FT_MUTABLE);

Comment on lines +173 to +178
const dsn::task_code load_from_private_log_level =
dsn::task_code::try_get(FLAGS_load_from_private_log_level, LPC_REPLICATION_LONG_LOW);
LOG_ERROR_PREFIX("unexpected load_from_private_log_level ({}), put it to default value "
"LPC_REPLICATION_LONG_LOW",
load_from_private_log_level);
fork(*_load_private, load_from_private_log_level, 0).link(*_ship);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const dsn::task_code load_from_private_log_level =
dsn::task_code::try_get(FLAGS_load_from_private_log_level, LPC_REPLICATION_LONG_LOW);
LOG_ERROR_PREFIX("unexpected load_from_private_log_level ({}), put it to default value "
"LPC_REPLICATION_LONG_LOW",
load_from_private_log_level);
fork(*_load_private, load_from_private_log_level, 0).link(*_ship);
auto dup_load_plog_task =
dsn::task_code::try_get(FLAGS_dup_load_plog_task, TASK_CODE_INVALID);
if (dup_load_plog_task == TASK_CODE_INVALID) {
dup_load_plog_task = LPC_REPLICATION_LONG_LOW;
LOG_ERROR_PREFIX("invalid dup_load_plog_task ({}), set it to LPC_REPLICATION_LONG_LOW",
FLAGS_dup_load_plog_task);
}
fork(*_load_private, dup_load_plog_task, 0).link(*_ship);

Comment on lines +52 to +54
"The level of load_from_private_log when doing a duplication.Be false means the "
"task level of replaing plog is low, otherwise the task level is common (We do "
"not recommend high level)");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"The level of load_from_private_log when doing a duplication.Be false means the "
"task level of replaing plog is low, otherwise the task level is common (We do "
"not recommend high level)");
"The task code for incremental loading from private logs while duplicating. Tasks with "
"TASK_PRIORITY_HIGH are not recommended.");

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants