-
Notifications
You must be signed in to change notification settings - Fork 10
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
Fix several bugs + add long-running scenario tests and build reports #12
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
CancellationTokens provided by Mjolnir (via the Timeout property for [Command]) weren't being passed through to CancellationToken parameters in Bifrost service calls. The Mjolnir token will now replace empty or null parameter values in Bifrost method calls if a CancellationToken parameter exists. If a token was explicitly provided to the call, it won't be replaced.
Change some test class visibilities and sleep times.
To facilitate controlling Riemann for all of Mjolnir during system testing.
Discovered a bug in Command where exeptions thrown directly from ExecuteAsync() were handled correctly, but exceptions thrown from its returned Task weren't marking failures in the circuit breaker (because we weren't awaiting the task in our try/catch. Fixed this bug and added a bunch of unit tests to make sure we have identical behavior regardless of where the exception is thrown from in ExecuteAsync(). Also updated the system test to dump its metrics to file per-scenario.
…s' into SystemTests Conflicts: Hudl.Mjolnir/Properties/AssemblyInfo.cs
robhruska
changed the title
Fix major circuit breaker bug, add long-running scenario tests and build reports
Fix several bugs and add long-running scenario tests and build reports
May 20, 2014
robhruska
changed the title
Fix several bugs and add long-running scenario tests and build reports
Fix several bugs + add long-running scenario tests and build reports
May 20, 2014
robhruska
added a commit
that referenced
this pull request
May 21, 2014
Fix several bugs + add long-running scenario tests and build reports
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New Tests
Adds Hudl.Mjolnir.SystemTests, which runs as a unit test, but is actually more of a long-running system test.
It's capable of serially running different scenarios to test and report on circuit breaker and thread pool behavior. Tests can hit a locally-running HTTP server whose response behavior can be configured.
Example tests:
For each test, Riemann metrics are intercepted and built into a Highcharts report:
(Download actual HTML here)
Minor notelets:
Fix errors not being counted by circuit breakers
After running a few scenarios and seeing a couple charts that didn't make sense to me, I discovered a pretty serious bug in Mjolnir where failures wouldn't get registered with the circuit breaker.
Since we weren't
await
ing the result ofExecuteAsync()
in our try/catch, we'd catch exceptions that were thrown directly fromExecuteAsync()
, but wouldn't see exceptions thrown from the Task that it returned. Our unit tests were only testing the former, so we didn't catch this.I fixed the issue and added a bunch of unit tests to make sure that we have the same behavior for exceptions coming out of
ExecuteAsync()
, both from the call itself and from its returned Task.Riemann via CommandContext
PLAT-112 - In order to better control Riemann during testing, I changed all of the components to grab their default Riemann instance from
CommandContext
instead of usingRiemannStats.Instance
. TheCommandContext
property can be changed, so changing it early in the app (before breakers and pools initialize) will cause them all to use it as well.Fix Cancellation with
[Command]
PLAT-21- CancellationTokens provided by Mjolnir (via the Timeout property for
[Command]
) weren't being passed through to CancellationToken parameters inBifrost service calls.
The Mjolnir token will now replace empty or null parameter values in
Bifrost method calls if a CancellationToken parameter exists. If a token
was explicitly provided to the call, it won't be replaced.
Avoid creating unnecessary
ConfigurableValue
sPLAT-113 - We'd create and then discard ConfigurableValues on every
GetCircuitBreaker()
call, which may have exacerbated an existing memory leak in our config library. Moved them into the delegate that gets called on breaker creation.Updated project README
Updated the README for this project to have examples and useful content.