Skip to content

fix: Fix potential deadlock in Snowflake destination#5553

Merged
kodiakhq[bot] merged 2 commits intomainfrom
fix-snowflake-deadlock
Dec 13, 2022
Merged

fix: Fix potential deadlock in Snowflake destination#5553
kodiakhq[bot] merged 2 commits intomainfrom
fix-snowflake-deadlock

Conversation

@hermanschaaf
Copy link
Copy Markdown
Contributor

@hermanschaaf hermanschaaf commented Dec 12, 2022

(Similar to: #5550)

If a worker returns an error, it will no longer be consuming from its writeChan, and in this case we should exit the loop early to avoid deadlock.

@hermanschaaf hermanschaaf requested review from a team and amanenk and removed request for a team December 12, 2022 15:47
@hermanschaaf hermanschaaf requested review from yevgenypats and removed request for amanenk December 12, 2022 15:54
erezrokah
erezrokah approved these changes Dec 12, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@erezrokah erezrokah left a comment

Choose a reason for hiding this comment

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

This looks good, but do we want to do something with the error returned? We can do this in a follow up PR

@hermanschaaf
Copy link
Copy Markdown
Contributor Author

hermanschaaf commented Dec 13, 2022

@erezrokah Maybe I'm missing something, but the error should get returned by the call to return eg.Wait() at the bottom, so not sure what more we need to do. I haven't tested it for Snowflake specifically, but in the BigQuery case this error bubbled up and got reported on the CLI.

@hermanschaaf hermanschaaf added the automerge Automatically merge once required checks pass label Dec 13, 2022
@kodiakhq kodiakhq bot merged commit ebf0eb8 into main Dec 13, 2022
@kodiakhq kodiakhq bot deleted the fix-snowflake-deadlock branch December 13, 2022 08:01
@cq-bot cq-bot mentioned this pull request Dec 13, 2022
Merged
@erezrokah
Copy link
Copy Markdown
Member

erezrokah commented Dec 13, 2022

@erezrokah Maybe I'm missing something, but the error should get returned by the call to return eg.Wait() at the bottom, so not sure what more we need to do. I haven't tested it for Snowflake specifically, but in the BigQuery case this error bubbled up and got reported on the CLI.

You're right, I missed that. No need for a follow up

kodiakhq bot pushed a commit that referenced this pull request Dec 13, 2022
@cq-bot
chore(main): Release plugins-destination-snowflake v1.0.6 (#5499)
b13fb29 
🤖 I have created a release *beep* *boop*
---


## [1.0.6](plugins-destination-snowflake-v1.0.5...plugins-destination-snowflake-v1.0.6) (2022-12-13)


### Bug Fixes

* **deps:** Update module github.com/cloudquery/plugin-sdk to v1.11.2 ([#5497](#5497)) ([c1876cf](c1876cf))
* **deps:** Update module github.com/cloudquery/plugin-sdk to v1.12.0 ([#5539](#5539)) ([fb71293](fb71293))
* Fix potential deadlock in Snowflake destination ([#5553](#5553)) ([ebf0eb8](ebf0eb8))

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@erezrokah erezrokah erezrokah approved these changes

@yevgenypats yevgenypats Awaiting requested review from yevgenypats

+1 more reviewer

@candiduslynx candiduslynx candiduslynx approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

automerge Automatically merge once required checks pass

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@hermanschaaf @erezrokah @candiduslynx @cq-bot