-
Notifications
You must be signed in to change notification settings - Fork 94
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
Prevent task events manager logging the same message more than once #5562
base: 8.2.x
Are you sure you want to change the base?
Conversation
16f966d
to
246c95e
Compare
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.
f1a8617
to
add64fe
Compare
@MetRonnie - I think that I fixed it. |
The original approach (adding messages to the activity log file and checking for them later seemed to break some of the workflow restart tests in ways that make me un-confident that they are safe changes. Scraping the log is horrible. Next thing to try: using the database - this isn't straightforward, because the database doesn't contain a table which would easily contain the task messages. |
add64fe
to
32930db
Compare
47e109f
to
7d9a83f
Compare
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.
Did some playing about with the UI
- Ran a workflow with a long-running task that does
cylc message
first thing - Stopped the workflow with
--now
- Restarted the workflow
The chip with the message did not appear on restart, whereas before it used to appear with "(polled)" appended
Not entirely surprised, but thank you for attempting it. |
57b0c98
to
f12f521
Compare
f12f521
to
001cc12
Compare
Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com>
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.
Got some suggestions for the tests and then I think this is good to go!
Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com>
@oliver-sanders are you waiting on me to do something with this? |
Kinda, but I need to take a closer look. I think this isn't skipping enough of the method, the problem is about more than just logging, e.g. it appears to running event handlers and spawning logic for duplicate messages? But I know that when you tried moving the |
I've had another look at this recently, and the notes on Matt Shin's original logic says
https://github.com/cylc/cylc-flow/pull/2157/files So it looks like Matt intended to have it work more simply than this and it's broken at some point. I'm going to reexamine this. :) |
Closes #5526
Deals with a bug was returning every message ever sent by a task on each poll.
(Draft pending further checks, to ensure that I haven't broken anything)
Check List
CONTRIBUTING.md
and added my name as a Code Contributor.setup.cfg
(andconda-environment.yml
if present).CHANGES.md
entry included if this is a change that can affect usersUpdate docs- this is a change to Cylc's logging.?.?.x
branch.