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.

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

Take advantage of Node's built-in streaming backpressure #77

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

erikap
Copy link

@erikap erikap commented Mar 4, 2024

This PR contains an attempt to take advantage of Node's built-in backpressure in streams (i.e. buffering in case of a slower consumer) instead of trying to manage it ourselves. It also includes an exponential backoff mechanism to retry failing requests.

This PR should not be merged as such. We've removed some code (like for example the paused flag) which may be considered a breaking change (although it looks like an internal to us) and didn't extensively test all different kinds of representations. Given that a new implementation is being worked on in https://github.com/TREEcg/ldes-client, it does not seem appropriate to us to further clean up and elaborate on this PR, but we wanted to indicate what our issues have been with this client so that they can possibly be taken into account in the new implementation.

Co-author: @madnifcent

@pietercolpaert
Copy link
Member

@ajuvercr Can you take a look to this at your earliest convenience and analyse whether there’s backpressure implemented in the new ldes-client?

@erikap Is the code in the PR functional and would you want us to release the last package of the deprecated event-stream-client with these changes?

@ajuvercr
Copy link

The new client certainly has backpressure, it uses the mdn streams interface and pauses everything after a member is found and resumes when a new member is expected.

If you need any other features in the new client that prevents you from changing, please let us know! @erikap @madnificent

Comment on lines +292 to +298
* TODO: There is an issue with the TREE metadata extractor
* changing the casing of the LDES IRI e.g.:
* https://semiceu.github.io/LinkedDataEventStreams/example.ttl#eventstream
* gets changed to:
* https://semiceu.github.io/linkeddataeventstreams/example.ttl#eventstream
* which prevents tree:views to get booked
*/

Choose a reason for hiding this comment

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

yikes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants