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

Compress log backup files by gzip #18645

Closed
wants to merge 4 commits into from

Conversation

brvphoenix
Copy link
Contributor

@brvphoenix brvphoenix commented Mar 2, 2023

Some operations (such as updating RSS schedulely) will make qbittorrent generate a lot of messages. But the qbittorrent will delete old backups only at startup previously. If qbittorrent is running for a long time, log backups will pile up over time.

The main changes in this PR are:

  1. Delete obsolete backups when a new backup is generated (previously only at startup). The backups will also be sorted.
  2. Allow to compress the log backups by gzip (disabled by default).

84143d487031282669c5a40eb3022747c6ff030c is a little hacking way to implement log compression with streams. I hesitate to add this because I find that the max allowed log file size is 1000MiB.

@glassez glassez added the Core label Mar 3, 2023
@glassez glassez self-assigned this Mar 12, 2023
Copy link
Member

@glassez glassez left a comment

Choose a reason for hiding this comment

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

I don't actually approve of the first commit (at least in its current form). It creates a cross dependencies between Application and FileLogger, which clearly cannot be seen as an improvement from a code structure perspective.

src/app/application.cpp Outdated Show resolved Hide resolved
src/app/filelogger.cpp Outdated Show resolved Hide resolved
src/app/filelogger.cpp Show resolved Hide resolved
@brvphoenix
Copy link
Contributor Author

brvphoenix commented Mar 13, 2023

I don't actually approve of the first commit (at least in its current form). It creates a cross dependencies between Application and FileLogger, which clearly cannot be seen as an improvement from a code structure perspective.

I'm not skilled in C++, so please forgive me for asking some ridiculous questions. Can I avoid cross dependencies if using the application interface class? Do you have any suggestions for this, keeping the original form, using a singleton or any other way?

@glassez
Copy link
Member

glassez commented Mar 13, 2023

Can I avoid cross dependencies if using the application interface class? Do you have any suggestions for this, keeping the original form, using a singleton or any other way?

I believe the easiest thing for all of us would be to revert this commit.

@glassez
Copy link
Member

glassez commented Mar 13, 2023

@brvphoenix
Isn't Read files by QDataStream a separate thing? I would strongly recommend to extract it into separate PR with explanation about its goals.
Also I would split "compressing" and "sorting" work unless they are really too interlinked.

src/app/filelogger.h Outdated Show resolved Hide resolved
src/app/filelogger.h Outdated Show resolved Hide resolved
src/app/application.cpp Outdated Show resolved Hide resolved
src/app/application.cpp Outdated Show resolved Hide resolved
Copy link
Member

@glassez glassez left a comment

Choose a reason for hiding this comment

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

@brvphoenix
I don't have time to review it all at once. I can either do it little by little, leaving some comments individually, or (if you don't like it that way) wait until I can review it in its entirety.

@brvphoenix brvphoenix force-pushed the compress branch 2 times, most recently from d8a126d to bbc9b77 Compare March 16, 2023 09:13
@brvphoenix
Copy link
Contributor Author

@brvphoenix I don't have time to review it all at once. I can either do it little by little, leaving some comments individually, or (if you don't like it that way) wait until I can review it in its entirety.

I'm OK with that ;).

@glassez
Copy link
Member

glassez commented Jun 23, 2023

@brvphoenix
Finally I found time to complete this review. I have a lot of comments about coding style, etc. But this is not the most important thing, it can be easily fixed.
I have a couple of more significant claims that I would really like to be resolved before it is approved:

  1. It would be better to unify the naming of backup files in such a way that the name of the compressed file differs only by adding a .gz extension to the name of the corresponding uncompressed one.
  2. I really don't like all this hassle with "sorting" backup files (and in fact, it's renaming a lot of files). It would be better for the backup file to remain untouched after its creation (up to its deletion, if configured). We could use the timestamp as a unique component of the file name.

src/app/application.cpp Show resolved Hide resolved
src/app/application.cpp Outdated Show resolved Hide resolved
src/app/filelogger.cpp Outdated Show resolved Hide resolved
src/app/filelogger.cpp Outdated Show resolved Hide resolved
src/app/filelogger.cpp Outdated Show resolved Hide resolved
src/app/filelogger.cpp Outdated Show resolved Hide resolved
src/app/filelogger.cpp Outdated Show resolved Hide resolved
src/app/filelogger.cpp Outdated Show resolved Hide resolved
src/app/filelogger.cpp Outdated Show resolved Hide resolved
src/app/filelogger.cpp Outdated Show resolved Hide resolved
test/testutilsgzip.cpp Outdated Show resolved Hide resolved
Chocobo1
Chocobo1 previously approved these changes Jun 30, 2023
Copy link
Member

@Chocobo1 Chocobo1 left a comment

Choose a reason for hiding this comment

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

Generally approved (without regarding the code)

src/base/utils/gzip.h Outdated Show resolved Hide resolved
@github-actions
Copy link

This PR is stale because it has been 60 days with no activity. This PR will be automatically closed within 7 days if there is no further activity.

@github-actions github-actions bot added the Stale label Oct 11, 2023
Copy link

This PR is stale because it has been 60 days with no activity. This PR will be automatically closed within 7 days if there is no further activity.

@github-actions github-actions bot added the Stale label Dec 12, 2023
@luzpaz
Copy link
Contributor

luzpaz commented Dec 13, 2023

Any traction here ?

@github-actions github-actions bot removed the Stale label Dec 14, 2023
Copy link

This PR is stale because it has been 60 days with no activity. This PR will be automatically closed within 7 days if there is no further activity.

@github-actions github-actions bot added the Stale label Feb 12, 2024
Copy link

This PR was closed because it has been stalled for some time with no activity.

@github-actions github-actions bot closed this Feb 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants