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

Support maxFileSize and archivedFileCount #1660

Merged
merged 2 commits into from Aug 4, 2016

Conversation

Projects
None yet
6 participants
@mattnelson
Contributor

mattnelson commented Aug 1, 2016

When the %i log format pattern is used with the maxFileSize and archivedFileCount properties, the logs are not rotated properly. It appear that the issue was that the triggering policy was not started.

-rw-r--r-- 1 dw dw  1368 Aug  1 18:02 dw_server-1.log.gz
-rw-r--r-- 1 dw dw  1807 Aug  1 18:01 dw_server-2.log.gz
-rw-r--r-- 1 dw dw  2151 Aug  1 18:00 dw_server-3.log.gz
-rw-r--r-- 1 dw dw  2170 Aug  1 18:00 dw_server-4.log.gz
-rw-r--r-- 1 dw dw  2149 Aug  1 18:00 dw_server-5.log.gz
-rw-r--r-- 1 dw dw  1384 Aug  1 18:00 dw_server_access-1.log.gz
-rw-r--r-- 1 dw dw  1378 Aug  1 18:00 dw_server_access-2.log.gz
-rw-r--r-- 1 dw dw  1419 Aug  1 18:00 dw_server_access-3.log.gz
-rw-r--r-- 1 dw dw  1403 Aug  1 18:00 dw_server_access-4.log.gz
-rw-r--r-- 1 dw dw  1410 Aug  1 18:00 dw_server_access-5.log.gz
-rw-r--r-- 1 dw dw 10101 Aug  1 18:00 dw_server_access.log
-rw-r--r-- 1 dw dw  3564 Aug  1 18:02 dw_server.log
[vagrant@server ~]$ for i in {1..100}; do curl -s http://localhost:8080/stuff > /dev/null; done
[vagrant@server ~]$ ll /opt/dw/server/logs/
total 76
-rw-r--r-- 1 dw dw  2950 Aug  1 18:03 dw_server-1.log.gz
-rw-r--r-- 1 dw dw  2934 Aug  1 18:03 dw_server-2.log.gz
-rw-r--r-- 1 dw dw  2963 Aug  1 18:03 dw_server-3.log.gz
-rw-r--r-- 1 dw dw  2967 Aug  1 18:03 dw_server-4.log.gz
-rw-r--r-- 1 dw dw  2940 Aug  1 18:03 dw_server-5.log.gz
-rw-r--r-- 1 dw dw  1582 Aug  1 18:03 dw_server_access-1.log.gz
-rw-r--r-- 1 dw dw  1589 Aug  1 18:03 dw_server_access-2.log.gz
-rw-r--r-- 1 dw dw  1387 Aug  1 18:03 dw_server_access-3.log.gz
-rw-r--r-- 1 dw dw  1384 Aug  1 18:00 dw_server_access-4.log.gz
-rw-r--r-- 1 dw dw  1378 Aug  1 18:00 dw_server_access-5.log.gz
-rw-r--r-- 1 dw dw   777 Aug  1 18:03 dw_server_access.log
-rw-r--r-- 1 dw dw 30130 Aug  1 18:03 dw_server.log
@coveralls

This comment has been minimized.

coveralls commented Aug 1, 2016

Coverage Status

Coverage increased (+0.003%) to 82.356% when pulling d40064b on mattnelson:file_limit_log_rotation into 1efbea3 on dropwizard:master.

assertThat(appender.getTriggeringPolicy()).isInstanceOf(SizeBasedTriggeringPolicy.class);
assertThat(appender.getTriggeringPolicy().isStarted());

This comment has been minimized.

@evnm

evnm Aug 1, 2016

Member

Adding this line without the above change to FileAppenderFactory doesn't result in a test failure for me. How did you confirm that the triggering policy isn't started?

This comment has been minimized.

@mattnelson

mattnelson Aug 2, 2016

Contributor

The assertion was wrong, I needed to add an isTrue. This caused the test to fail before making the change.

Looks like this was added in logback-1.1.5[1] to make sure the triggering policy is started. Since the appenders are not started as part of the testing, the checks are never executed.

[1] qos-ch/logback@1af14db#diff-168f8cbb3bd8b03acc60eb147007d385R51

final SizeBasedTriggeringPolicy<E> triggeringPolicy = new SizeBasedTriggeringPolicy<>();
triggeringPolicy.setMaxFileSize(String.valueOf(maxFileSize.toBytes()));
triggeringPolicy.setContext(context);
triggeringPolicy.start();

This comment has been minimized.

@evnm

evnm Aug 1, 2016

Member

Shouldn't triggeringPolicy.start(); be invoked below in the else block too?

This comment has been minimized.

@mattnelson

mattnelson Aug 2, 2016

Contributor

Posted under wrong thread, reposting in the correct context.

I think there are some other issues[1] with the implementation in the else block. It seems like the %d-%i configuration should've never been supported. I believe there will need more intrusive changes in order to get a more robust rolling/triggering policy in those workflows.

[1] http://jira.qos.ch/browse/LOGBACK-1127?focusedCommentId=17110&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-17110
[2] http://jira.qos.ch/browse/LOGBACK-1143

@nickbabcock

This comment has been minimized.

Contributor

nickbabcock commented Aug 2, 2016

Is this related to #1173?

@mattnelson

This comment has been minimized.

Contributor

mattnelson commented Aug 2, 2016

Yes, This is related to #1173.

@mattnelson

This comment has been minimized.

Contributor

mattnelson commented Aug 2, 2016

Fixed the assertions on 37036a4

@evnm

This comment has been minimized.

Member

evnm commented Aug 2, 2016

LGTM

@coveralls

This comment has been minimized.

coveralls commented Aug 2, 2016

Coverage Status

Coverage increased (+0.005%) to 82.358% when pulling 37036a4 on mattnelson:file_limit_log_rotation into 1efbea3 on dropwizard:master.

@jplock jplock added the improvement label Aug 3, 2016

@jplock jplock added this to the 1.1.0 milestone Aug 3, 2016

@nickbabcock nickbabcock merged commit ab65a32 into dropwizard:master Aug 4, 2016

@mattnelson mattnelson deleted the mattnelson:file_limit_log_rotation branch Aug 8, 2016

arteam added a commit that referenced this pull request Oct 28, 2016

Support maxFileSize and archivedFileCount (#1660)
* Support max file with size log rotation

(cherry picked from commit ab65a32)

arteam added a commit that referenced this pull request Oct 28, 2016

@arteam arteam modified the milestones: 1.0.3, 1.1.0 Oct 31, 2016

@joschi joschi referenced this pull request Jul 19, 2018

Closed

log generation issue #2435

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment