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

Refactor childStats requestRestartPermission algorithm #689

Merged

Conversation

rodrigovidal
Copy link
Contributor

No description provided.

rogeralsing added a commit that referenced this pull request Feb 28, 2015
…ermission

Refactor childStats requestRestartPermission algorithm
@rogeralsing rogeralsing merged commit 5da2746 into akkadotnet:dev Feb 28, 2015
@HCanber
Copy link
Contributor

HCanber commented Mar 1, 2015

Why was this changed? For non trivial "refactorings" I'd like to see an explanation as we might need to be able to go back and see why things change.

In this case, the changes aren't just a refactoring, this PR has altered the algorithm. Unfortunately it introduced several bugs, and needs to be reverted, as support for negative values have been completely removed.

The values for maxNrOfRetries and withinTimeMilliseconds are specified when creating AllForOneStrategy and OneForOneStrategy. From the docs for these constructors:
-- maxNrOfRetries: the number of times a child actor is allowed to be restarted, negative value means no limit, if the limit is exceeded the child actor is stopped.
-- withinTimeMilliseconds: duration in milliseconds of the time window for maxNrOfRetries, negative values means no window.

These calls now does the incorrect thing:

RequestRestartPermission(-1,-1)
RequestRestartPermission(10,-1)
RequestRestartPermission(-1, 10)

@rodrigovidal
Copy link
Contributor Author

@HCanber Yes, I was fooled by the variables names. I considered that maxNrOfRetries and withinTimeMilliseconds could not be negative. I was wrong. Do we have tests for this? I would be glad to help adding it.

@rogeralsing sorry about this mistake.

@Aaronontheweb
Copy link
Member

@rodrigovidal looks like we don't have a test that would have caught this issue - would you mind adding one?

@rodrigovidal
Copy link
Contributor Author

@Aaronontheweb Sure! I'll do it 👍

@Aaronontheweb
Copy link
Member

awesome, thanks @rodrigovidal

@HCanber
Copy link
Contributor

HCanber commented Mar 2, 2015

@rodrigovidal 👍

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.

4 participants