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

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

Conversation

maxi297
Copy link
Contributor

@maxi297 maxi297 commented Jan 19, 2023

What

In order to validate if the number of requests was limited, we want to be able to know how many pages and slices were queries. We already had the information for the pages but didn't for the slices. This PR addresses the slices part.

How

Emit a log message when a new slice is being started and catch this log message to create the slices properly

Recommended reading order

  1. airbyte-cdk/python/airbyte_cdk/sources/abstract_source.py: how the slice message is logged
  2. airbyte-connector-builder-server/connector_builder/impl/default_api.py: how we use this log message to create the slices on the connector-builder server

🚨 User Impact 🚨

@flash1293, will this be supported by the UI? Although not a breaking API change, there were some assumptions in the backend that there will always be only one slice.

@maxi297 maxi297 requested a review from a team as a code owner January 19, 2023 16:49
@octavia-squidington-iv octavia-squidington-iv added the CDK Connector Development Kit label Jan 19, 2023
@maxi297 maxi297 temporarily deployed to more-secrets January 19, 2023 16:52 — with GitHub Actions Inactive
@maxi297 maxi297 temporarily deployed to more-secrets January 19, 2023 16:53 — with GitHub Actions Inactive
@flash1293
Copy link
Contributor

I need to test this - I think there is some code in there to handle multiple slices but as you noted it's not used at the moment. But it's not blocking for this PR, we can also fix it as a follow-up.

@maxi297 maxi297 temporarily deployed to more-secrets January 19, 2023 17:34 — with GitHub Actions Inactive
@maxi297 maxi297 temporarily deployed to more-secrets January 19, 2023 17:35 — with GitHub Actions Inactive
@maxi297
Copy link
Contributor Author

maxi297 commented Jan 19, 2023

@girarda if you are interested

@maxi297 maxi297 temporarily deployed to more-secrets January 19, 2023 18:25 — with GitHub Actions Inactive
@maxi297
Copy link
Contributor Author

maxi297 commented Jan 19, 2023

There is no test that validates that ManifestDeclarativeSource will configure the logger to debug. Is this something we should have?

@maxi297 maxi297 requested a review from girarda January 19, 2023 19:23
Copy link
Contributor

@girarda girarda left a comment

Choose a reason for hiding this comment

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

couple of questions that maybe span outside of the scope of the PR. lgtm 🚢

airbyte-cdk/python/airbyte_cdk/sources/abstract_source.py Outdated Show resolved Hide resolved
* [ISSUE #20771] set flag when limit requests reached

* [ISSUE #20771] assert proper value on test read objects __init__
@maxi297 maxi297 temporarily deployed to more-secrets January 20, 2023 16:04 — with GitHub Actions Inactive
@maxi297 maxi297 temporarily deployed to more-secrets January 20, 2023 16:05 — with GitHub Actions Inactive
@maxi297 maxi297 temporarily deployed to more-secrets January 20, 2023 18:47 — with GitHub Actions Inactive
@maxi297 maxi297 temporarily deployed to more-secrets January 20, 2023 18:48 — with GitHub Actions Inactive
@maxi297 maxi297 merged commit cf63ee5 into issue-20771_limit-number-of-requests-to-endpoint Jan 20, 2023
@maxi297 maxi297 deleted the issue-20771_adding-slices-to-read-response branch January 20, 2023 18:52
maxi297 added a commit that referenced this pull request 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CDK Connector Development Kit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants