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

[connector builder] limit the number of requests submitted to the endpoint #20771

Closed
girarda opened this issue Dec 21, 2022 · 2 comments
Closed

Comments

@girarda
Copy link
Contributor

girarda commented Dec 21, 2022

Tell us about the problem you're trying to solve

We should limit the number of requests the connector builder can send as part of a "test read" to limit how expensive a read is.

As a result:

  • you'll have to wait at the UI a long time
  • we could use too much API quota
  • In cloud it could be exploited

Describe the solution you’d like

A way to limit the number of requests submitted to an API endpoint because of a single "test read"

Acceptance criteria

  • The server has a hard limit on the number of requests per slice and number of slices (initial proposal: 5)
  • If a read call attempts to do more than the limit of allowed requests, it will stop and return the data gathered so far. A flag on the response is set so the UI can react to the fact the read command got limited
  • If this flag is set, the UI shows a warning callout that only a certain number of requests has been made

Implementation details

  • This will require updating the HttpStream so it stops paginating after X requests
  • (to validate) this will require update the AbstractSource so it stops iterating over the stream slices after X requests

Open questions

  • Do we also need this limit on OSS?
    • To keep things simple IMHO we shouldn't have different behavior on OSS
  • Should the be configurable (with a max)?
    • Not configurable for now IMHO
  • What should the max be?
    • IMHO going with 5 - 10 for now would be sufficient
  • What about secondary requests like for oauth/session token auth?
    • Ignore for the scope of this ticket
    • Create separate issue for consolidating request handling
  • What about complex slice/page combinations where 10 is too little and it wouldn't hit the second slice / be not representative in some way?
    • Apply the limit not the overall number of requests, but
      • Limit requests per slice
      • Limit slices
@flash1293
Copy link
Contributor

Related to that and also important:

  • Fail faster if API is not available
  • Right now default behavior is doing really long exponential backoff which makes sense during production but not while iterating
  • Maybe we can solve this in a similar way?

@flash1293
Copy link
Contributor

Side note: Should we cancel the read once the upstream connection gets closed? Might be separate issue

@maxi297 maxi297 self-assigned this Jan 17, 2023
maxi297 added a commit that referenced this issue Jan 18, 2023
maxi297 added a commit that referenced this issue Jan 19, 2023
maxi297 added a commit that referenced this issue Jan 19, 2023
maxi297 added a commit that referenced this issue Jan 19, 2023
maxi297 added a commit that referenced this issue Jan 20, 2023
maxi297 added a commit that referenced this issue Jan 20, 2023
* [ISSUE #20771] set flag when limit requests reached

* [ISSUE #20771] assert proper value on test read objects __init__
maxi297 added a commit that referenced this issue Jan 20, 2023
* [ISSUE #20771] adding slices to connector builder read request

* [ISSUE #20771] formatting

* [ISSUE #20771] set flag when limit requests reached (#21619)

* [ISSUE #20771] set flag when limit requests reached

* [ISSUE #20771] assert proper value on test read objects __init__

* [ISSUE #20771] code review and fix edge case
maxi297 added a commit that referenced this issue Jan 20, 2023
maxi297 added a commit that referenced this issue Jan 24, 2023
maxi297 added a commit that referenced this issue Jan 24, 2023
#21525)

* [ISSUE #20771] limiting the number of requests performed to the backend without flag

* [ISSUE #20771] code reviewing my own code

* [ISSUE #20771] adding ABC to paginator

* [ISSUE #20771] format code

* [ISSUE #20771] adding slices to connector builder read request (#21605)

* [ISSUE #20771] adding slices to connector builder read request

* [ISSUE #20771] formatting

* [ISSUE #20771] set flag when limit requests reached (#21619)

* [ISSUE #20771] set flag when limit requests reached

* [ISSUE #20771] assert proper value on test read objects __init__

* [ISSUE #20771] code review and fix edge case

* [ISSUE #20771] fix flake8 error

* [ISSUE #20771] code review

* 🤖 Bump minor version of Airbyte CDK

* to run the CI
@maxi297 maxi297 closed this as completed Jan 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants