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

(Do Not Merge) Source S3: Bump Resource Allocation For Check & Discover #53204

Closed
wants to merge 8 commits into from

Conversation

agarctfi
Copy link
Contributor

@agarctfi agarctfi commented Feb 6, 2025

What

For: https://github.com/airbytehq/oncall/issues/6959
Users have a lot of files, causing a timeout during schema discovery.

How

We bumped the memory and CPU limits to see if this would allow discover to finish. We will make a dev image from this and pin their connection.

Review guide

User Impact

Can this PR be safely reverted and rolled back?

  • YES 💚
  • NO ❌

Copy link

vercel bot commented Feb 6, 2025

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 Feb 6, 2025 9:57pm

@agarctfi agarctfi changed the title Source S3: Bump Resource Allocation For Check & Discover (Do Not Merge) Source S3: Bump Resource Allocation For Check & Discover Feb 6, 2025
- jobType: discover_schema
resourceRequirements:
memory_limit: 4Gi
memory_request: 4Gi
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean to do this on the source? Destinations do not run discover schema.

Also this has infra implications doing this large of a bump. Specifically for SME customers who run on smaller nodes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry to drag you into this, but @colesnodgrass did you say we strip these requirements for SME? If so, this would not affect SME.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will remove this one from the destinations, thanks! We won't be releasing it; we'll just be testing it on the cloud through dev images since these specific users have been blocked for quite some time.

- jobType: discover_schema
resourceRequirements:
memory_limit: 4Gi
memory_request: 4Gi
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto.

- jobType: check_connection
resourceRequirements:
memory_limit: 4096mi
memory_request: 4096mi
Copy link
Contributor

Choose a reason for hiding this comment

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

Checks should be have very low resource requirements. I'd consider this a source bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From my understanding, Source S3, could be an outlier given the size of the file the user has on their source.

@agarctfi
Copy link
Contributor Author

agarctfi commented Feb 6, 2025

It doesn't seem like this solved the issue so closing this PR for now

@agarctfi agarctfi closed this Feb 6, 2025
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