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

[windows] record restarts on a specific timeframe #1664

Merged
merged 1 commit into from
Jun 9, 2015

Conversation

yannmh
Copy link
Member

@yannmh yannmh commented Jun 8, 2015

Drop the global count of restarts in favor of a timeframe based one.

@yannmh yannmh added the windows label Jun 8, 2015
@yannmh yannmh self-assigned this Jun 8, 2015
@yannmh yannmh added this to the 5.4.0 milestone Jun 8, 2015
def restart(self):
self._count_restarts += 1
if self._count_restarts >= self._MAX_RESTARTS:
if not self._can_restart():
servicemanager.LogInfoMsg(
"%s reached the limit of restarts. Not restarting..." % self._name)
Copy link
Member

Choose a reason for hiding this comment

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

To ease a potential debug, could you add the number of restarts (or tries), max number of restarts, and maybe the time window ?
So something like:

"{0} reached the limit of restarts ({1} tries during the last {3}s (max authorized: {2})). Not restarting...".format(self._name, len(self._restarts), self._restarts[0] - time.time()) ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea, thanks.

@yannmh yannmh force-pushed the yann/win-max-restart-timeframe branch from 1cbaf83 to 203e6ea Compare June 9, 2015 14:11
self._name = name
self._process = process
self._count_restarts = 0
self._restarts = deque([])
self._MAX_RESTARTS = max_restarts
Copy link
Member

Choose a reason for hiding this comment

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

I don't really like this way (not your PR, but close), having a "dynamic" constant. Maybe you could instead have a DEFAULT_MAX_RESTARTS = 5, max_restarts=None, and self._max_restarts = max_restarts or self.DEFAULT_MAX_RESTARTS: what do you think ?

@degemer
Copy link
Member

degemer commented Jun 9, 2015

Nice improvement!
LGTM, if you could test a custom build before merging it, it would be perfect!

Drop the global count of restarts in favor of a timeframe based one.
@yannmh yannmh force-pushed the yann/win-max-restart-timeframe branch from 203e6ea to 49f7cb3 Compare June 9, 2015 14:28
yannmh added a commit that referenced this pull request Jun 9, 2015
[windows] record restarts on a specific timeframe
@yannmh yannmh merged commit 5e84e5a into master Jun 9, 2015
@yannmh yannmh deleted the yann/win-max-restart-timeframe branch June 9, 2015 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants