Skip to content

ApiListener: Reset m_LogMessageCount when rotating #9999

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

Merged
merged 1 commit into from
Feb 15, 2024

Conversation

yhabteab
Copy link
Member

@yhabteab yhabteab commented Feb 9, 2024

Closing and re-opening that very same log file shouldn't reset the counter, otherwise some log files may exceed the max limit per file as their offset indicator is reset each time they are re-opened.

ref/IP/48306

@cla-bot cla-bot bot added the cla/signed label Feb 9, 2024
@yhabteab yhabteab added area/distributed Distributed monitoring (master, satellites, clients) bug Something isn't working ref/IP labels Feb 9, 2024
@yhabteab yhabteab requested a review from julianbrost February 9, 2024 15:52
Closing and re-opening that very same log file shouldn't reset the
counter, otherwise some log files may exceed the max limit per file as
their offset indicator is reset each time they are re-opened.
@yhabteab yhabteab force-pushed the reset-log-message-count-correctly branch from 8a4c7ad to 6e66cd9 Compare February 9, 2024 17:06
Copy link
Contributor

@julianbrost julianbrost left a comment

Choose a reason for hiding this comment

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

Closing and re-opening that very same log file shouldn't reset the counter, otherwise some log files may exceed the max limit per file as their offset indicator is reset each time they are re-opened.

Reopening the same file without rotation is something that actually happens when sending the replay log:

CloseLogFile();
if (count == -1 || count > 50000) {
OpenLogFile();

So there's indeed a situation where those files can become larger than intended without this change.

@julianbrost julianbrost added this to the 2.15.0 milestone Feb 15, 2024
@julianbrost julianbrost added the consider backporting Should be considered for inclusion in a bugfix release label Feb 15, 2024
@julianbrost julianbrost merged commit 7d1c887 into master Feb 15, 2024
@julianbrost julianbrost deleted the reset-log-message-count-correctly branch February 15, 2024 16:06
@Al2Klimov Al2Klimov added backported Fix was included in a bugfix release and removed consider backporting Should be considered for inclusion in a bugfix release labels Feb 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/distributed Distributed monitoring (master, satellites, clients) backported Fix was included in a bugfix release bug Something isn't working cla/signed ref/IP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants