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 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
SQS-412 | Active Orders Query: SSE #518
SQS-412 | Active Orders Query: SSE #518
Changes from 2 commits
cc3d892
81d5aa6
5688eb0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Who is then responsible for closing it to prevent leakage?
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.
Since channels are not like files it's fine to keep those open. Closing is necessary when we want to inform client that there will be no values send, which is simple to achieve with one sender, but difficult with mulitple senders, because sending to close channel would cause a panic. In our scenario sender will have always some values to sent, thus I think adding synchronization to close a channel would only introduce unecessary complexity without clear benefit.
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.
Is there any way we could reuse this in
GetActiveOrders
to reduce code duplication?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 think atm code is quite opimised it for duplication: we're reusing common data structures; I think further optmisations would add not significant improvements vs time spent finding right abstractions due smalll differences between
fetchOrders
andGetActiveOrders
. For example infetchOrders
we skip empty results, have additional logging specifc for this method.