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

fix(connector-builder): add flag to disable cache #45095

Merged
merged 4 commits into from
Sep 9, 2024

Conversation

lazebnyi
Copy link
Collaborator

@lazebnyi lazebnyi commented Sep 3, 2024

What

disable caching when running from the builder

How

add the disable_cache option to the ModelToComponentFactory and pass this value during the initialization of the requester

Review guide

  1. model_to_component_factory.py
  2. connector_builder_handler.py

User Impact

No

Can this PR be safely reverted and rolled back?

  • YES 💚
  • NO ❌

Copy link

vercel bot commented Sep 3, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
airbyte-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 9, 2024 1:09pm

@octavia-squidington-iii octavia-squidington-iii added the CDK Connector Development Kit label Sep 3, 2024
@lazebnyi lazebnyi requested a review from girarda September 3, 2024 11:15
@maxi297
Copy link
Contributor

maxi297 commented Sep 3, 2024

I think this can be fine as a temporary fix but it will change the behavior of connectors between the Connector Builder and an actual run. This is an example where there is a cache issue and disabling the cache would make the Connector Builder have the right behavior for the customer during test reads but the sync would have missing records.

Would there be an easy way to call HttpClient.clear_cache when this is a test read instead?

Copy link
Contributor

@bnchrch bnchrch left a comment

Choose a reason for hiding this comment

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

This seems straightforward to me. Will defer to @girarda and @maxi297 if correct.

Copy link
Contributor

@maxi297 maxi297 left a comment

Choose a reason for hiding this comment

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

I'm fine with this as a temporary fix. The longer term fix would be https://github.com/airbytehq/airbyte-internal-issues/issues/9650

@maxi297
Copy link
Contributor

maxi297 commented Sep 3, 2024

@lazebnyi Before merging, can we validate these cases? I fear we might still have caches even with your change

@lazebnyi
Copy link
Collaborator Author

lazebnyi commented Sep 4, 2024

@maxi297 we created the stream component with the updated stream configuration you mentioned , but we disabled caching right before creating the instance (line 836 - model_to_component_factory.py)
so, from my perspective, this approach seems fine

@maxi297
Copy link
Contributor

maxi297 commented Sep 4, 2024

This approach also seems fine to me but I would feel safer with a test to confirm that substreams are also affected by disabling the cache. This will also serve as a good safety net against regression

@lazebnyi
Copy link
Collaborator Author

lazebnyi commented Sep 9, 2024

/approve-regression-tests

Check job output.

✅ Approving regression tests

@lazebnyi lazebnyi merged commit 41b8585 into master Sep 9, 2024
36 checks passed
@lazebnyi lazebnyi deleted the lazebnyi/source-fix-cache-in-connector-builder branch September 9, 2024 19:51
@lazebnyi
Copy link
Collaborator Author

lazebnyi commented Sep 9, 2024

local e2e tests passed successfully

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