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

Quiet loop logs; improve loop intervals #43

Merged
merged 14 commits into from
Feb 3, 2021
Merged

Quiet loop logs; improve loop intervals #43

merged 14 commits into from
Feb 3, 2021

Conversation

jlowin
Copy link
Member

@jlowin jlowin commented Aug 14, 2020

Thanks for contributing to Prefect Server!

Please describe your work and make sure your PR:

  • adds new tests (if appropriate)
  • adds a changelog entry to the changes/ directory (if appropriate)

Note that your PR will not be reviewed unless these two boxes are checked.

What does this PR change?

This PR:

  • Quiets debug logs from loop services to heartbeat no more than once every 5 minutes. For rapid services, the noise from heartbeating every loop makes working with logs difficult
  • Improves the loop interval calculation so it starts from the beginning of each period not the end. If a service takes one hour to run and has a loop interval of 24 hours, this means it will now start every 24 hours instead of every 25 hours.
  • Adds a warning if the loop interval is shorter than the execution time (which would lead to too-rapid execution without pauses)

@jlowin jlowin requested a review from cicdw as a code owner August 14, 2020 14:42
@codecov-commenter
Copy link

codecov-commenter commented Aug 14, 2020

Codecov Report

Merging #43 into master will increase coverage by 0.23%.
The diff coverage is 88.23%.

@jlowin
Copy link
Member Author

jlowin commented Aug 18, 2020

@cicdw made the following changes:

  • added a stop() method to facilitate testing and interactive use (otherwise the services run infinitely)
  • added tests, including for the situation you described
  • removed the random starting delay - overengineered complexity with no clear benefit ✅
  • turned on asyncio debug mode

@cicdw
Copy link
Member

cicdw commented Dec 30, 2020

Closing this as we've made (most of) these adjustments elsewhere.

@cicdw cicdw closed this Dec 30, 2020
@jlowin jlowin reopened this Jan 28, 2021
@jlowin jlowin requested a review from zanieb as a code owner January 28, 2021 22:29
@jlowin
Copy link
Member Author

jlowin commented Jan 28, 2021

Reopening following offline conversation that the remaining enhancements in this PR may be desirable

Copy link
Member

@cicdw cicdw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor request to reuse an existing attribute, otherwise this LGTM

@@ -40,21 +44,19 @@ def __init__(self, loop_seconds: Union[float, int] = None):
self.loop_seconds = float(loop_seconds)
self.name = type(self).__name__
self.logger = utilities.logging.get_logger(self.name)
self._stop_running = False
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we already have an _is_running attribute that you should probably reuse instead

Copy link
Member

@cicdw cicdw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - this will be a nice QOL improvement

@cicdw cicdw merged commit fe60c04 into master Feb 3, 2021
@cicdw cicdw deleted the loop-service branch February 3, 2021 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants