Skip to content

Destination redshift: remove standard inserts #38886

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jun 18, 2024
Merged

Destination redshift: remove standard inserts #38886

merged 2 commits into from
Jun 18, 2024

Conversation

@edgao
Copy link
Contributor

@edgao edgao commented Jun 3, 2024 •
edited
Loading

@vercel
Copy link

vercel bot commented Jun 3, 2024 •
edited
Loading

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 Jun 18, 2024 5:49pm

@edgao Graphite App
Copy link
Contributor Author

edgao commented Jun 3, 2024 •
edited
Loading

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @edgao and the rest of your teammates on Graphite Graphite

@edgao edgao marked this pull request as ready for review June 3, 2024 22:02
@edgao edgao requested a review from a team as a code owner June 3, 2024 22:02
@edgao edgao force-pushed the edgao/redshift_no_more_standard_inserts branch from 0f21678 to f871777 Compare June 3, 2024 22:23
@edgao edgao mentioned this pull request Jun 3, 2024
Merged
evantahler
evantahler reviewed Jun 4, 2024
View reviewed changes
Copy link
Contributor

@evantahler evantahler left a comment

Choose a reason for hiding this comment

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

This will be a breaking change - don't forget that metadata update

@edgao edgao force-pushed the edgao/redshift_no_more_standard_inserts branch from f871777 to 4fddc2c Compare June 4, 2024 15:45
@edgao edgao mentioned this pull request Jun 4, 2024
Merged
@edgao edgao force-pushed the edgao/redshift_no_more_standard_inserts branch from 4fddc2c to 21f196b Compare June 4, 2024 21:00
@octavia-squidington-iii octavia-squidington-iii added the area/documentation Improvements or additions to documentation label Jun 4, 2024
@edgao edgao added the breaking-change Don't merge me unless you are ready. label Jun 4, 2024
evantahler
evantahler approved these changes Jun 4, 2024
View reviewed changes
@edgao edgao force-pushed the edgao/redshift_no_more_standard_inserts branch 2 times, most recently from de741dc to 3882716 Compare June 5, 2024 15:57
gisripa
gisripa reviewed Jun 5, 2024
View reviewed changes
gisripa
gisripa reviewed Jun 5, 2024
View reviewed changes
...byte/integrations/destination/redshift/RedshiftS3StagingInsertDestinationAcceptanceTest.java
@Execution(ExecutionMode.SAME_THREAD)
@Disabled
public class RedshiftS3StagingInsertDestinationAcceptanceTest extends RedshiftDestinationAcceptanceTest {
public class RedshiftS3StagingInsertDestinationAcceptanceTest extends JdbcDestinationAcceptanceTest {
Copy link
Contributor

@gisripa gisripa Jun 5, 2024

Choose a reason for hiding this comment

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

😕 why does a disabled test has large diff, rename of some other file ?

Copy link
Contributor Author

@edgao edgao Jun 12, 2024

Choose a reason for hiding this comment

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

collapsing the class hierarchy - there used to be base test + standard insert test + s3 test, now there's just the s3 test

gisripa
gisripa reviewed Jun 5, 2024
View reviewed changes
gisripa
gisripa approved these changes Jun 5, 2024
View reviewed changes
Copy link
Contributor

@gisripa gisripa left a comment

Choose a reason for hiding this comment

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

lgtm

This was referenced Jun 5, 2024
Merged
Closed
@edgao edgao force-pushed the edgao/redshift_no_more_standard_inserts branch from 3882716 to a31cc6c Compare June 7, 2024 00:06
@edgao edgao force-pushed the edgao/redshift_no_more_standard_inserts branch from a31cc6c to 3b48c99 Compare June 7, 2024 15:15
@edgao edgao force-pushed the edgao/redshift_no_more_standard_inserts branch from 3b48c99 to ae12995 Compare June 12, 2024 18:16
@edgao edgao force-pushed the edgao/redshift_no_more_standard_inserts branch from ae12995 to ff5f8a5 Compare June 12, 2024 19:39
@edgao edgao force-pushed the edgao/redshift_no_more_standard_inserts branch from ff5f8a5 to 0be1c21 Compare June 14, 2024 18:06
@edgao edgao force-pushed the edgao/redshift_no_more_standard_inserts branch from 0be1c21 to 007829b Compare June 17, 2024 17:30
@edgao edgao force-pushed the edgao/redshift_no_more_standard_inserts branch from 007829b to a6564b8 Compare June 18, 2024 17:34
@edgao edgao force-pushed the edgao/redshift_no_more_standard_inserts branch from a6564b8 to e4e8169 Compare June 18, 2024 17:44
@edgao edgao merged commit 0e949ae into master Jun 18, 2024
@edgao edgao deleted the edgao/redshift_no_more_standard_inserts branch June 18, 2024 19:57
This was referenced Jun 21, 2024
Merged
Merged
This was referenced Jul 2, 2024
Merged
Merged
Merged
@assaadhjb
Copy link
Contributor

assaadhjb commented Jul 8, 2024 •
edited
Loading

Hello @edgao , Airbyte Cloud user here, May I ask in the roadmap where this has been determined?

Screenshot 2024-07-08 at 14 28 23

Thanks 🙏

This was referenced Jul 8, 2024
Open
Open
@evantahler evantahler mentioned this pull request Jul 11, 2024
Closed
2 tasks
@assaadhjb
Copy link
Contributor

assaadhjb commented Jul 11, 2024 •
edited
Loading

Issue consequent to this risen here: #41031

@edgao
Copy link
Contributor Author

edgao commented Jul 11, 2024

sorry, I missed your comment earlier - this was mostly an internal decision, hence us setting a somewhat longer upgrade timeline. Standard inserts usage is unfortunately too low to justify the amount of engineering and support time it takes, and we decided that rather than try to build newer features on both code paths, it made more sense to just stop supporting the inserts mode

@edgao edgao mentioned this pull request Jul 26, 2024
Merged
@edgao edgao mentioned this pull request Aug 14, 2024
Closed
@mycotics
Copy link

mycotics commented Oct 3, 2025 •
edited
Loading

This was a terrible idea.

You removed standard inserts without introducing the option to use iam_roles for COPY query.
not only iam_role is the best practice (and the only options in some industries) and if one uses 2FA which requires the access key and secret to be temporary, makes the connector unusable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

3 more reviewers

@evantahler evantahler evantahler approved these changes

@gisripa gisripa gisripa approved these changes

@stephane-airbyte stephane-airbyte stephane-airbyte approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area/connectors Connector related issues area/documentation Improvements or additions to documentation breaking-change Don't merge me unless you are ready. connectors/destination/redshift

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@edgao @assaadhjb @mycotics @evantahler @gisripa @stephane-airbyte @octavia-squidington-iii