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

Remove maxlines option for file logger #5282

Merged
merged 1 commit into from
Nov 7, 2018

Conversation

lafriks
Copy link
Member

@lafriks lafriks commented Nov 6, 2018

To support this feature it requires scanning whole log file on every ssh git operation that is not sensible. Remove this feature as there is already option for file size that is more reasonable to use and other log file rotation options.

Should fix #4450

@lafriks lafriks added pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! backport/v1.6 labels Nov 6, 2018
@lafriks lafriks added this to the 1.7.0 milestone Nov 6, 2018
@micw
Copy link
Contributor

micw commented Nov 6, 2018

Looks good. I'd use something around 100 MB which comes closer to the initial 1 million lines.

@bkcsoft bkcsoft added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 6, 2018
@codecov-io
Copy link

Codecov Report

Merging #5282 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5282      +/-   ##
==========================================
- Coverage   37.38%   37.37%   -0.01%     
==========================================
  Files         312      312              
  Lines       46327    46313      -14     
==========================================
- Hits        17320    17311       -9     
+ Misses      26525    26520       -5     
  Partials     2482     2482
Impacted Files Coverage Δ
modules/setting/setting.go 48.06% <100%> (-0.13%) ⬇️
modules/log/file.go 42.73% <100%> (-0.68%) ⬇️
modules/process/manager.go 76.81% <0%> (-4.35%) ⬇️
models/repo_list.go 64.55% <0%> (+1.26%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 078c404...8c3df19. Read the comment docs.

@lafriks lafriks added issue/critical This issue should be fixed ASAP. If it is a PR, the PR should be merged ASAP type/miscellaneous labels Nov 6, 2018
@micw
Copy link
Contributor

micw commented Nov 6, 2018

@lafriks milestone 1.7.0? Since it's a fix for a performance-critical issue, I'd appreciate to see it in 1.6.

edit: sorry, did not see the backport label.

@adelowo
Copy link
Member

adelowo commented Nov 6, 2018

Isn't 256MB still large enough?

Copy link
Contributor

@zeripath zeripath left a comment

Choose a reason for hiding this comment

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

This seems sensible. Logrotate and log4j do not have this option either

@bkcsoft bkcsoft added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Nov 6, 2018
Copy link
Member

@bkcsoft bkcsoft left a comment

Choose a reason for hiding this comment

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

I would deprecate it and print a warning that it no longer does anything. Otherwise LGTM

@bkcsoft bkcsoft added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Nov 6, 2018
@sapk
Copy link
Member

sapk commented Nov 6, 2018

@bkcsoft the breaking label should indicate this in the changelog.

@lafriks
Copy link
Member Author

lafriks commented Nov 7, 2018

It's not documented anywhere and it would print on every git ssh operation, so I don't really see how we could deprecate it

@lafriks lafriks merged commit 8f8ff5a into go-gitea:master Nov 7, 2018
@lafriks lafriks deleted the fix/logging_mem_usage branch November 7, 2018 04:51
lafriks added a commit to lafriks-fork/gitea that referenced this pull request Nov 7, 2018
@lafriks lafriks added the backport/done All backports for this PR have been created label Nov 7, 2018
@micw
Copy link
Contributor

micw commented Nov 7, 2018

@lafriks Shouldn't I see this backport in https://github.com/go-gitea/gitea/commits/release/v1.6 now?

@lafriks
Copy link
Member Author

lafriks commented Nov 7, 2018

@micw there is other PR for backport to release/v1.6 branch #5287

@lunny
Copy link
Member

lunny commented Nov 7, 2018

For further consideration, maybe we should use other maintained-well log library rather than we maintained a new one.

@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/done All backports for this PR have been created issue/critical This issue should be fixed ASAP. If it is a PR, the PR should be merged ASAP lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! type/miscellaneous
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Serv command consumes all server resources
8 participants