-
Notifications
You must be signed in to change notification settings - Fork 1
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(event-streams): CPU Utilization #176
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Negative LOC is good :)
Fauna.Test/Integration.Tests.cs
Outdated
@@ -449,7 +449,7 @@ public Task StreamThrowsWithBadRequest() | |||
} | |||
}); | |||
|
|||
Assert.AreEqual("BadRequest: Bad Request", ex?.Message); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How come this changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, if this is expected I'd like to understand why. We should also rename the test appropriately and assert the correct exception type–hopefully we're not throwing FaunaException anywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't returning the response on failed requests like I was before, but I've put it back to how it was. Initially I thought this was an improvement because we didn't have to create an exception from the response body but I realized it's better to early return as I had it before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🥳
Fauna.Test/Integration.Tests.cs
Outdated
@@ -449,7 +449,7 @@ public Task StreamThrowsWithBadRequest() | |||
} | |||
}); | |||
|
|||
Assert.AreEqual("BadRequest: Bad Request", ex?.Message); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, if this is expected I'd like to understand why. We should also rename the test appropriately and assert the correct exception type–hopefully we're not throwing FaunaException anywhere?
SummarySummary
CoverageFauna - 81.5%
Delta Summary
|
Description
Update
OpenStream
to use aBlockingCollection
Motivation and context
We had a tight loop in the
EventListener
that was causing high CPU utilizationHow was the change tested?
Integration tests and
Sandbox app
Screenshots (if appropriate):
Before
After
Change types
Checklist:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.