Skip to content
This repository has been archived by the owner on Feb 27, 2023. It is now read-only.

log: not rotate log file when startup #926

Merged
merged 1 commit into from
Sep 18, 2019

Conversation

lowzj
Copy link
Member

@lowzj lowzj commented Sep 18, 2019

Ⅰ. Describe what this PR did

Ⅱ. Does this pull request fix one issue?

fixes part of : #921

Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗?)

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

@codecov-io
Copy link

codecov-io commented Sep 18, 2019

Codecov Report

Merging #926 into master will decrease coverage by <.01%.
The diff coverage is 14.28%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #926      +/-   ##
==========================================
- Coverage   46.75%   46.75%   -0.01%     
==========================================
  Files         112      112              
  Lines        6651     6649       -2     
==========================================
- Hits         3110     3109       -1     
  Misses       3291     3291              
+ Partials      250      249       -1
Impacted Files Coverage Δ
cmd/dfdaemon/app/init.go 0% <ø> (ø) ⬆️
...t/core/downloader/p2p_downloader/p2p_downloader.go 0% <0%> (ø) ⬆️
pkg/dflog/log.go 50.5% <100%> (-0.99%) ⬇️
supernode/daemon/mgr/scheduler/manager.go 22.6% <0%> (+0.68%) ⬆️

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 9114104...de058d6. Read the comment docs.

@yeya24
Copy link
Collaborator

yeya24 commented Sep 18, 2019

What will happen if the log file exceed the max file size 40MB?

logrus.Warnf("request piece result:%v", response)
if code == constants.CodeSourceError {
p2p.cfg.BackSourceReason = config.BackSourceReasonSourceError
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

Redundant else.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not redundant. It makes the warn log only outputted when getting an unexpected response code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, you are right. Sorry for not have a look at the code carefully.

@@ -61,7 +61,7 @@ func WithLogFile(f string) Option {
if logger := getLumberjack(l); logger == nil {
l.SetOutput(&lumberjack.Logger{
Filename: f,
MaxSize: 20, // mb
MaxSize: 40, // mb
Copy link
Contributor

Choose a reason for hiding this comment

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

make it configurable? WDYT? And so do with the MaxBackups.

Copy link
Member Author

Choose a reason for hiding this comment

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

agree. There's already a function named WithMaxSizeMB to configure MaxSize. It's better to open a new pr to implement this especially make the supernode's log configurable.

@starnop
Copy link
Contributor

starnop commented Sep 18, 2019

LGTM.

@starnop
Copy link
Contributor

starnop commented Sep 18, 2019

What will happen if the log file exceed the max file size 40MB?

@yeya24 FYI. https://github.com/natefinch/lumberjack

image

@starnop starnop merged commit 74d7ccb into dragonflyoss:master Sep 18, 2019
starnop added a commit to starnop/Dragonfly that referenced this pull request Nov 27, 2019
log: not rotate log file when startup
inoc603 pushed a commit to inoc603/Dragonfly that referenced this pull request Dec 23, 2019
log: not rotate log file when startup
@lowzj lowzj deleted the fix-p2p-download branch May 3, 2020 06:19
sungjunyoung pushed a commit to sungjunyoung/Dragonfly that referenced this pull request May 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants