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

WriteController: Remove redundant setting delay reports with single db #831

Merged
merged 5 commits into from
Feb 13, 2024

Conversation

Yuval-Ariel
Copy link
Contributor

@Yuval-Ariel Yuval-Ariel commented Feb 8, 2024

The WC currently reports to the log whenever a new delay request is handled (even if its the same rate).
These msgs are redundant in cases where the WC is only shared with 1 db and hence 1 logger since similar msgs are reported in ColumnFamilyData::RecalculateWriteStallConditions.

Currently it looks like :

2024/02/11-12:34:41.726676 7213 [WARN] [/write_controller.cc:153] WC setting delay of 178892456, client_id: 0x563f8a2e8f60, client rate: 178892456
2024/02/11-12:34:41.726684 7213 [WARN] [/column_family.cc:1235] [default] Stalling writes because we have 12 level-0 files rate 178892456

This PR removes the msg from the write_controller (the first) when there is only 1 logger to report to.

@Yuval-Ariel Yuval-Ariel self-assigned this Feb 8, 2024
@CLAassistant
Copy link

CLAassistant commented Feb 8, 2024

CLA assistant check
All committers have signed the CLA.

@Yuval-Ariel Yuval-Ariel changed the title WIP: write controller report setting delay only with multiple loggers WriteController: Remove redundant setting delay reports with single db Feb 11, 2024
@udi-speedb
Copy link
Contributor

@Yuval-Ariel - Why not avoid logging in the WC to the reporter's logger?
This would achieve what this PR is for as well as have a single log record (instead of 2) in the reporter's log.

@Yuval-Ariel
Copy link
Contributor Author

@Yuval-Ariel - Why not avoid logging in the WC to the reporter's logger? This would achieve what this PR is for as well as have a single log record (instead of 2) in the reporter's log.

@udi-speedb , what you're suggesting wouldnt work when a logger is shared between two dbs. the db that initated the request will get a msg but the other one wouldnt.

@udi-speedb
Copy link
Contributor

@Yuval-Ariel - Why not avoid logging in the WC to the reporter's logger? This would achieve what this PR is for as well as have a single log record (instead of 2) in the reporter's log.

@udi-speedb , what you're suggesting wouldnt work when a logger is shared between two dbs. the db that initated the request will get a msg but the other one wouldnt.

Wouldn't the reporter get a log message from its own column_family.cc code while the other(s) would log the WC's message?

@Yuval-Ariel
Copy link
Contributor Author

@Yuval-Ariel - Why not avoid logging in the WC to the reporter's logger? This would achieve what this PR is for as well as have a single log record (instead of 2) in the reporter's log.

@udi-speedb , what you're suggesting wouldnt work when a logger is shared between two dbs. the db that initated the request will get a msg but the other one wouldnt.

Wouldn't the reporter get a log message from its own column_family.cc code while the other(s) would log the WC's message?

its the same logger. so both dbs will get any msg

@udi-speedb
Copy link
Contributor

udi-speedb commented Feb 12, 2024

OK, but if there are multiple delay sources (cf-s, WBM) wouldn't you miss the min rate and only see the new rate?

@Yuval-Ariel
Copy link
Contributor Author

OK, but if there are multiple delay sources (cf-s, WBM) wouldn't you miss the min rate and only see the new rate?

we would still get the msg from the cf that initiated the delay

@Yuval-Ariel Yuval-Ariel merged commit b2c5bd5 into main Feb 13, 2024
18 checks passed
@Yuval-Ariel Yuval-Ariel deleted the yuval/remove-redundant-wc-reports branch February 13, 2024 10:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants