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

FileTarget - Dynamic archive mode with more strict file-mask for cleanup #2524

Merged

Conversation

snakefoot
Copy link
Contributor

@snakefoot snakefoot commented Jan 18, 2018

Avoid too aggressive cleanup when having multiple log-archives in the same folder.

Make the NLog 4.5 archive logic more userfriendly.

@snakefoot snakefoot force-pushed the FileTargetArchiveDynamicStrict branch 2 times, most recently from 883bf61 to c253710 Compare January 18, 2018 18:27
@snakefoot snakefoot force-pushed the FileTargetArchiveDynamicStrict branch from c253710 to fcc5960 Compare January 18, 2018 18:28
@codecov
Copy link

codecov bot commented Jan 18, 2018

Codecov Report

Merging #2524 into master will decrease coverage by <1%.
The diff coverage is 85%.

@@           Coverage Diff           @@
##           master   #2524    +/-   ##
=======================================
- Coverage      82%     82%   -<1%     
=======================================
  Files         323     323            
  Lines       23375   23423    +48     
  Branches     2920    2931    +11     
=======================================
+ Hits        19060   19093    +33     
- Misses       3545    3559    +14     
- Partials      770     771     +1

@304NotModified
Copy link
Member

Make the NLog 4.5 archive logic more userfriendly.

Could you elaborate on this?

more userfriendly

👍 👍

@304NotModified 304NotModified added the enhancement Improvement on existing feature label Jan 19, 2018
@snakefoot
Copy link
Contributor Author

Could you elaborate on this?

Just look at the change of the unit-test, and you should see the problem this PR is trying to fix.

LogFile1 has this filename: log.txt
LogFile2 has this filename: log-other.txt

The automatic wildcard for LogFile1 will become log*.txt, which will also cleanup LogFile2 files.

@304NotModified 304NotModified added this to the 4.5 rc5 milestone Jan 19, 2018
@304NotModified
Copy link
Member

is this a case we could unit test? (the first red part)

image

@304NotModified
Copy link
Member

any idea if this could fix the unstable test " NLog.UnitTests.Targets.PlainFileTargetTests.SimpleFileDeleteTest(concurrentWrites: False, keepFileOpen: True, networkWrites: False, forceManaged: True, forceMutexConcurrentWrites: False, optimizeBufferReuse: True) [FAIL]"

See https://travis-ci.org/NLog/NLog/builds/329033070 (branch master...)

@snakefoot
Copy link
Contributor Author

is this a case we could unit test? (the first red part)

It is just a safe-guard to ensure the double-string-iteration doesn't go bananas. You are welcome to think up a unit test.

@snakefoot
Copy link
Contributor Author

snakefoot commented Jan 19, 2018

any idea if this could fix the unstable test SimpleFileDeleteTest

Well my "fix" is to skip the keepfileopen-config for this test on MONO, as they seem very on unstable on Travis right now (Seems the FileWatcher is not doing its job very well, just like all config-auto-reload tests are skipped on Travis).

@304NotModified 304NotModified merged commit 9ed6dad into NLog:master Jan 19, 2018
@snakefoot snakefoot deleted the FileTargetArchiveDynamicStrict branch January 29, 2018 17:28
@304NotModified 304NotModified added the file-archiving Issues with archiving with the file target label Aug 26, 2019
@snakefoot snakefoot modified the milestones: 4.5 rc5, 4.5 Aug 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvement on existing feature file-archiving Issues with archiving with the file target file-target
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants