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

Rest & Batch consolidation #16

Merged
merged 18 commits into from
Dec 15, 2022
Merged

Rest & Batch consolidation #16

merged 18 commits into from
Dec 15, 2022

Conversation

alejoberardino
Copy link
Contributor

This one has very deep changes. I'll keep it as short as I can:

  • Rest and BatchWarming transports have been consolidated into a new Http transport.
    • All requests will be processed in the background.
    • New interface allows for an arbitrary number of requests, to contemplate use cases like:
      • One request to the DP per each request to the EA.
      • A single batch request to the DP for all requests to the EA.
      • Any combination of requests, for example:
        • One for symbol batches, another for id batches.
        • Partial batches when the number of items to request would exceed the max for a batch endpoint.
  • A new helper dependency Requester is added to centralize the execution of request to the Data Provider:
    • Makes rate limiting easy by processing requests sequentially and sleeping when capacity is reached.
    • Does not handle weights or priorities of any kind for now (good enough for most DPs).
    • Handles retries by adding requests back into the queue if they fail.
    • Coalesces requests by using a map alongside a linked list for the queue to ensure that repeated requests are not added as new ones.
  • Background executor no longer relies on the rate limiter to sleep between calls. Now it only sleeps for a very small time (to allow for transports to implement their own timings)

Copy link
Contributor

@boxhock boxhock left a comment

Choose a reason for hiding this comment

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

Looking really good, and looking forward to implementing EAs with these changes!

Just had a few small comments

Copy link
Contributor

@boxhock boxhock left a comment

Choose a reason for hiding this comment

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

Finally spent some more time on the last (big) file

@alejoberardino alejoberardino merged commit 6e8c925 into main Dec 15, 2022
@alejoberardino alejoberardino deleted the feature/requester branch December 15, 2022 16:31
@github-actions
Copy link
Contributor

🚀 Successfully created version bump PR: #15

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

Successfully merging this pull request may close these issues.

3 participants