-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
SimpleScheduler - fix CronTrigger#evaluate() #17763
Conversation
mkouba
commented
Jun 8, 2021
- resolves Scheduler is not triggered or triggered 2 times randomly with native image #17724
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. CI was not happy with the first run though, I relaunched it.
GitHub has problems: https://www.githubstatus.com/ :-( |
The change is described in #17724 (comment) |
@mkouba VERY bad idea to rename the logger in the same commit. I have conflicts... When you want to backport a fix, please do NOT make cosmetic changes that don't bring anything to the plate. |
I fixed the conflicts myself by saying that for next time. |
@gsmet I'm really sorry about that. I tried to make a diff locally and saw no conflicts. But this change was not only about renaming the logger, I also added/removed some log messages. Was it really the rename itself that caused the conflict? |
Yes. Some methods are not in 1.13 and the fact that the logger name had changed made them present in the diff as they are logging something. There's a good chance they would have been ignored otherwise. It wasn't too hard to fix it but for a more complex patch, that could have been a major pain thus my message. |