Skip to content

Chore(Source S3): Bump to CDK v5 (release candidate) - #46298

Merged
Aaron ("AJ") Steers (aaronsteers) merged 50 commits into
masterfrom
aj/source-s3/bump-to-latest-cdk
Oct 14, 2024
Merged

Chore(Source S3): Bump to CDK v5 (release candidate)#46298
Aaron ("AJ") Steers (aaronsteers) merged 50 commits into
masterfrom
aj/source-s3/bump-to-latest-cdk

Conversation

@aaronsteers

@aaronsteers Aaron ("AJ") Steers (aaronsteers) commented Oct 1, 2024

Copy link
Copy Markdown
Member

What

Builds on:

Prereq to:

Adds CDK version bump to v5.

How

Fixed Untested Codepaths and Brittle Entrypoint Relationship

Prior to this PR, all of the CAT-related failures were un untested code paths. So, it was a 10-20 minute airbyte-ci feedback loop to reproduce issues. This PR adds a number of test structures, and it moves all code logic into test blocks that are on the critical path. The exception is "create()" which requires a catalog input. I put notes in the code that we should try to refactor this so that a source can be created without the catalog input and then passed the catalog after initialization - when it is being passed anyway.

There are two new class methods added on SourceS3. The first is SourceS3.create() which adds as a Source factory, reducing the complexity of creating a source without introducing any breaking changes to the constructor itself. This was a key point of failure for CAT tests, and the updated code is now in the test pass.

The second new class method is SourceS3.launch(). As proposed in the Python grooming session a few weeks back, this change allows Sources to launch them, removing the brittle and unnecessarily brittle relationship between Entrypoint and Connector classes, allowing connectors to launch themselves.

One-Click CAT Tests.

I added test_acceptance.py whose logic should eventually should be pushed into the CDK. This takes a 10-20 minute test loop and makes it a 10 second one. You can re-run any failed test directly, and you can also step-debug through any test. Lastly, we can now easily get test coverage metrics which are specific to the acceptance test and CAT scenarios. Any code not called by CAT scenario can be considered untested for those mapped scenarios - and might actually be dead code. More on this in a coming Loom.

Review guide

User Impact

Can this PR be safely reverted and rolled back?

  • YES 💚
  • NO ❌

@vercel

vercel Bot commented Oct 1, 2024

Copy link
Copy Markdown

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

1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
airbyte-docs ⬜️ Ignored (Inspect) Visit Preview Oct 14, 2024 7:52pm

@octavia-squidington-iii Octavia Squidington III (octavia-squidington-iii) added the area/documentation Improvements or additions to documentation label Oct 1, 2024
@aaronsteers
Aaron ("AJ") Steers (aaronsteers) marked this pull request as ready for review October 1, 2024 22:41
@aaronsteers

Aaron ("AJ") Steers (aaronsteers) commented Oct 1, 2024

Copy link
Copy Markdown
Member Author

/approve-regression-tests

Check job output.

✅ Approving regression tests

@aaronsteers

This comment was marked as outdated.

@octavia-squidington-iii Octavia Squidington III (octavia-squidington-iii) added the CDK Connector Development Kit label Oct 2, 2024
@aaronsteers

Aaron ("AJ") Steers (aaronsteers) commented Oct 8, 2024

Copy link
Copy Markdown
Member Author

For context: this PR was spun up because of headaches with breaking changes unrelated to the larger refactoring here:

And this PR replaces the first attempt here:

I've bumped all of these down to draft status, fwiw. The order of operations once I'm off of oncall rotation is:

  1. Get the CDK bumped.
  2. Fix the concurrent CDK bug, which otherwise prevents file-based CDK to use concurrency in specific instances.
  3. Optionally: Perform additional refactoring, if helpful.

Since we're expecting a decent perf boost from 1 and 2, number 3 is not "unstoppable" on its own.

Added context: The larger refactoring investigation also looked into more pervasive changes to the overall paradigm - specifically eliminating unnecessary record loops when we can delegate down to lower-level APIs. Since the CDK is very record-centric, and since there were plenty of gains to achieve with the version bump, plus just fixing what was broken, we pushed the larger refactor out-of-scope.

Some of this context also shared in Slack. And I've created a new channel #proj-s3-performance-boosts for focused conversations on this initiative.

@aaronsteers

Aaron ("AJ") Steers (aaronsteers) commented Oct 9, 2024

Copy link
Copy Markdown
Member Author

/format-fix

Format-fix job started... Check job output.

✅ Changes applied successfully. (e294041)

@aaronsteers

Aaron ("AJ") Steers (aaronsteers) commented Oct 14, 2024

Copy link
Copy Markdown
Member Author

/approve-regression-tests

Check job output.

✅ Approving regression tests

auto-merge was automatically disabled October 14, 2024 10:20

Pull request was closed

@aaronsteers

Aaron ("AJ") Steers (aaronsteers) commented Oct 14, 2024

Copy link
Copy Markdown
Member Author

/approve-regression-tests "no breaking config, catalog, or state change expected"

Check job output.

✅ Approving regression tests

@aaronsteers

Aaron ("AJ") Steers (aaronsteers) commented Oct 14, 2024

Copy link
Copy Markdown
Member Author

Catherine Noll (@clnoll), Natik Gadzhi (@natikgadzhi) - This is now passing tests and ready for final review. Once approved, we can use it for progressive rollout.

Caveat: I am not up to speed yet with how progressive rollouts should be configured. We may need to apply some changes here - such as using an "rc" version number.

Comment thread airbyte-integrations/connectors/source-s3/pyproject.toml Outdated

@alafanechere Augustin (alafanechere) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just dropping comments if you want to make this version a RC for progressive rollout

Comment thread airbyte-integrations/connectors/source-s3/metadata.yaml Outdated
@aaronsteers

Aaron ("AJ") Steers (aaronsteers) commented Oct 14, 2024

Copy link
Copy Markdown
Member Author

/approve-regression-tests "no breaking config, catalog, or state change expected"

Check job output.

✅ Approving regression tests

@aaronsteers Aaron ("AJ") Steers (aaronsteers) changed the title Chore(Source S3): Bump to CDK v5 (new) Chore(Source S3): Bump to CDK v5 (release candidate) Oct 14, 2024
@aaronsteers

Aaron ("AJ") Steers (aaronsteers) commented Oct 14, 2024

Copy link
Copy Markdown
Member Author

/format-fix

Format-fix job started... Check job output.

🟦 Job completed successfully (no changes).

@aaronsteers

Aaron ("AJ") Steers (aaronsteers) commented Oct 14, 2024

Copy link
Copy Markdown
Member Author

/approve-regression-tests "no breaking config, catalog, or state change expected"

Check job output.

✅ Approving regression tests

Comment thread airbyte-integrations/connectors/source-s3/metadata.yaml Outdated
Co-authored-by: Natik Gadzhi <natik@respawn.io>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

With the isReleaseCandidate, as long as CI is green, I'm happy!

@aaronsteers

Aaron ("AJ") Steers (aaronsteers) commented Oct 14, 2024

Copy link
Copy Markdown
Member Author

/approve-regression-tests "no breaking config, catalog, or state change expected"

Check job output.

✅ Approving regression tests

@aaronsteers

Aaron ("AJ") Steers (aaronsteers) commented Oct 14, 2024

Copy link
Copy Markdown
Member Author

/approve-regression-tests "no breaking config, catalog, or state change expected"

Check job output.

✅ Approving regression tests

@aaronsteers
Aaron ("AJ") Steers (aaronsteers) deleted the aj/source-s3/bump-to-latest-cdk branch October 14, 2024 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/connectors Connector related issues area/documentation Improvements or additions to documentation CDK Connector Development Kit connectors/source/s3

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants