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
chore: adding metric observable to be able to support async update #964
chore: adding metric observable to be able to support async update #964
Changes from 3 commits
dda9c18
78af295
734d58e
2d298eb
5e6d8ba
4539e51
e6a307b
6ec9b73
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.
Can we use
setImmediate(subscriber, value)
to avoid calling subscribers synchronouslyThere 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.
Why do think it would be a problem if we call them synchronously. The common pattern for such things is to make it synchronous. Also if user is calling "next" I would assume that this is the moment it should be updated, including the time as well, using setImmediate the time won't be accurate. I also think that this might cause some unexpected behaviour, if for any reason you would like to also call "collect" after you update values. I would be rather against that.
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.
Just to avoid blocking the event loop for too long if there are many subscribers to update, or if the subscribers take too long to process a value. A user may also
subscribe
to updates, not just us, and the user is unlikely to know their callback is in a performance sensitive contextThere 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.
The "hidden" async operation will prevent user from calling
collect
immediately after updating the values, and time for updating the value will also be different then moment of calling next. Besides that "blocking loop" would last as long as callback in "standard way" anyway.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.
@mayurkale22 @vmarchaud whats your thoughts on that ?
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.
@vmarchaud setImmediate will delay update - it won't be updated asap as spec says
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 don't find any requirement on specific time requirement, could you give a link please ?
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.
@vmarchaud you wrote
the spec state that the update path should be as fast as possible
so setImmediate won't update asap but it will update in next cycleThere 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 see, the whole spec state:
I interpret
short code path
from a performance requirement and not as a time requirement. So usingsetImmediate
would be a better solution for meThere 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'm strongly against it but if majority want's setImmediate with it's all consequences fine for me, @mayurkale22 you also vote for setImmediate ?