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

Trigger policy before log #290

Closed
wants to merge 1 commit into from
Closed

Trigger policy before log #290

wants to merge 1 commit into from

Conversation

snaggen
Copy link

@snaggen snaggen commented Oct 28, 2022

Re-arrange rolling file appender, so that it applies the policy
before it writes the next log row.

Fixes #289

@estk
Copy link
Owner

estk commented Oct 30, 2022

Seems good, can you add two tests:

  • Test that it fixes the problem with the date triggers
  • Test that it doesn't break size triggers

One thing to note here is that the .process method does the rolling, looking at the code I think we won't have a race with the background rotation stuff but it will be good to have a test that uses background_rotation, and triggers the roll and immediately writes another log to be sure it doesn't get lost in the churn

@snaggen
Copy link
Author

snaggen commented Nov 2, 2022

Busy with other stuff right now, but I will try to add some tests when I find the time

@estk
Copy link
Owner

estk commented Nov 17, 2022

Thanks @snaggen, really appreciate your work here.

@brotskydotcom
Copy link

Hi @snaggen, would you be OK if I tried adding the tests that @estk is asking for? I have a time-based trigger that needs your fix so I'd like to help get it merged.

@snaggen
Copy link
Author

snaggen commented Nov 29, 2022

Go ahead

@bconn98
Copy link
Collaborator

bconn98 commented Nov 30, 2023

@brotskydotcom Any luck with your tests?

@Dirreke
Copy link
Contributor

Dirreke commented Dec 4, 2023

@brotskydotcom Any luck with your tests?

I think it's just like what I have done in my PR #296, which you said that should be changed back. @bconn98

@brotskydotcom
Copy link

Hey folks, sorry to take a while to get back on this. I had pulled this fix into a separate branch that I've been using in another product and never got back to these tests. I can probably try getting the tests done this week. But @Dirreke are you saying you already have tests for this? Or that you have already integrated this fix into your PR which is waiting to be pulled?

@estk
Copy link
Owner

estk commented Dec 4, 2023

I'm assuming this feature was necessary to get the first line of 3rd party time-loggers working, therefore closing in favor of #296 , lets continue discussion over there.

@estk estk closed this Dec 4, 2023
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.

Trigger is invoked after log, causing date based triggers to log one line in the wrong file
5 participants