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.

Sign up for GitHub

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

Jump to bottom

Add circuit breaker exponential backoff support #4350

Merged
merged 1 commit into from Aug 22, 2020
Merged

Add circuit breaker exponential backoff support #4350

merged 1 commit into from Aug 22, 2020

Conversation

ismaelhamed
Copy link
Member

@ismaelhamed ismaelhamed commented Mar 19, 2020 •
edited

Issue: #21036

This feature has existed in the JVM since 2016, and allows to specify an exponential backoff for the reset timeout.

@Aaronontheweb Aaronontheweb added this to the 1.4.4 milestone Mar 19, 2020
@Aaronontheweb Aaronontheweb self-requested a review March 19, 2020 13:35
@ismaelhamed
Copy link
Member Author

ismaelhamed commented Mar 19, 2020 •
edited

@Aaronontheweb I forgot to mention: this introduces a breaking change which will force plugins and such to recompile, since the CircuitBreaker's constructor signature has changed, requiring now a scheduler.

@ismaelhamed
Copy link
Member Author

I'll review the failing tests

@ismaelhamed ismaelhamed force-pushed the circuit-breaker-21061 branch 4 times, most recently from bfa8ebb to c530c05 Compare March 29, 2020 11:00
@Aaronontheweb Aaronontheweb modified the milestones: 1.4.4, 1.4.5 Mar 31, 2020
@ismaelhamed ismaelhamed force-pushed the circuit-breaker-21061 branch 2 times, most recently from 1f1323e to e9f26fd Compare April 9, 2020 17:57
@Aaronontheweb Aaronontheweb modified the milestones: 1.4.5, 1.4.6 Apr 29, 2020
@Aaronontheweb Aaronontheweb modified the milestones: 1.4.6, 1.4.7 May 12, 2020
@Aaronontheweb Aaronontheweb modified the milestones: 1.4.7, 1.4.8 May 26, 2020
@Aaronontheweb Aaronontheweb modified the milestones: 1.4.8, 1.4.9 Jun 17, 2020
@Aaronontheweb Aaronontheweb modified the milestones: 1.4.9, 1.4.10 Jul 21, 2020
@Aaronontheweb
Copy link
Member

@ismaelhamed this needs a rebase - but I'll take a look at it for the 1.4.11 release. I promise <3

@Aaronontheweb Aaronontheweb modified the milestones: 1.4.10, 1.4.11 Aug 20, 2020
@ismaelhamed
Copy link
Member Author

That's gonna take me a minute, there are some conflicts

@Aaronontheweb
Copy link
Member

Yep, sorry about that - my fault for not moving on it sooner

Aaronontheweb
Aaronontheweb reviewed Aug 21, 2020
View reviewed changes
Copy link
Member

@Aaronontheweb Aaronontheweb left a comment

Choose a reason for hiding this comment

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

Has some breaking changes but I think this are relatively minor, since the CircuitBreaker is mostly an internal implementation detail for Akka.Persistence journals

src/core/Akka.API.Tests/CoreAPISpec.ApproveCore.approved.txt
@@ -3884,22 +3884,27 @@ namespace Akka.Pattern
}
public class CircuitBreaker
{
public CircuitBreaker(int maxFailures, System.TimeSpan callTimeout, System.TimeSpan resetTimeout) { }
public CircuitBreaker(Akka.Actor.IScheduler scheduler, int maxFailures, System.TimeSpan callTimeout, System.TimeSpan resetTimeout) { }
public CircuitBreaker(Akka.Actor.IScheduler scheduler, int maxFailures, System.TimeSpan callTimeout, System.TimeSpan resetTimeout, System.TimeSpan maxResetTimeout, double exponentialBackoffFactor) { }
Copy link
Member

Choose a reason for hiding this comment

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

Bit of a breaking change here, but I suppose this really can't be done without passing in the IScheduler under the circumstances - right?

Copy link
Member Author

Choose a reason for hiding this comment

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

The ResetTimeout is done via the scheduler.

src/core/Akka.API.Tests/CoreAPISpec.ApproveCore.approved.txt Outdated
@@ -3915,9 +3920,12 @@ namespace Akka.Pattern
}
public class OpenCircuitException : Akka.Actor.AkkaException
{
public OpenCircuitException() { }
public OpenCircuitException(System.TimeSpan remainingDuration) { }
Copy link
Member

Choose a reason for hiding this comment

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

This TimeSpan has a functional purpose, I take it?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is only used during the tests, to do some assertions. See #L315

src/core/Akka.Tests/Pattern/CircuitBreakerSpec.cs
@@ -303,6 +303,29 @@ public void Should_Transition_To_Half_Open_When_Reset_Timeout( )
Assert.True( InterceptExceptionType<TestException>( ( ) => breaker.Instance.WithCircuitBreaker( () => Task.Factory.StartNew( ThrowException ) ).Wait( ) ) );
Assert.True( CheckLatch( breaker.HalfOpenLatch ) );
}

[Fact(DisplayName = "An asynchronous circuit breaker that is open should increase the reset timeout after it transits to open again")]
Copy link
Member

Choose a reason for hiding this comment

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

LGTM

@Aaronontheweb Aaronontheweb merged commit 65f3044 into akkadotnet:dev Aug 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Aaronontheweb Aaronontheweb Aaronontheweb left review comments

Assignees
No one assigned
Labels
akka-actor akka-persistence
Projects
None yet
Milestone
1.4.11
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@ismaelhamed @Aaronontheweb