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

CreateNormalizedTable: lift loop, make it based on separate connector #1280

Merged
merged 4 commits into from
Feb 13, 2024

Conversation

serprex
Copy link
Contributor

@serprex serprex commented Feb 13, 2024

Snowflake was sending a heartbeat for each table, BQ was using HeartbeatRoutine, clickhouse wasn't sending heartbeats. Unify on HeartbeatRoutine

Move to a separate connector interface so s3/eventhubs etc don't need to implement the start/abort/finish methods as stubs too

Also introduce a generalized GetConnectorAs, use internally for existing functions

Also introduce a generalized GetConnectorAs, use internally for existing functions
@serprex serprex force-pushed the heartbeat-in-setup-normalized branch from 91e623f to 0840060 Compare February 13, 2024 15:14
}
defer conn.AbortSetupNormalizedTables(ctx, tx)
Copy link
Contributor

Choose a reason for hiding this comment

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

defer aborts feels like we will always Abort. I'm wondering if there is a way to say this RollbackIfError or something,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Considered MaybeRollback but went with Cleanup to keep it a general "defer cleanup of whatever your tx object is"

@iskakaushik iskakaushik merged commit 2b0a2cc into main Feb 13, 2024
6 of 7 checks passed
@iskakaushik iskakaushik deleted the heartbeat-in-setup-normalized branch February 13, 2024 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants