fix(destinations): Propagate write errors#461
Closed
erezrokah wants to merge 2 commits intocloudquery:mainfrom
Closed
fix(destinations): Propagate write errors#461erezrokah wants to merge 2 commits intocloudquery:mainfrom
erezrokah wants to merge 2 commits intocloudquery:mainfrom
Conversation
erezrokah
commented
Dec 5, 2022
| s.Logger.Error().Err(err).Msg("failed to wait") | ||
| return fmt.Errorf("plugin write failed: %w", err) | ||
| } | ||
| return ctx.Err() |
Member
Author
There was a problem hiding this comment.
ctx.Err() returns context canceled when the write fails, which is not the error we want
⏱️ Benchmark resultsComparing with 3fecc61
|
5 tasks
hermanschaaf
suggested changes
Dec 6, 2022
| close(resources) | ||
| if err := eg.Wait(); err != nil { | ||
| s.Logger.Error().Err(err).Msg("failed to unmarshal resource. failed to wait for plugin") | ||
| return fmt.Errorf("plugin write failed: %w", err) |
Contributor
There was a problem hiding this comment.
I think we are losing the information from the original JSON unmarshal error here now - that's why the previous implementation only logged the Wait error and then returned the original JSON unmarshal error. Maybe we should keep it that way? I'm open to other options too, but we shouldn't drop that error without logging it at least
Member
Author
There was a problem hiding this comment.
Yeah, I'm not sure. I actually think we should return a list of errors, but that's a bigger change.
I think the Write error is the first one that occurred so it makes more sense to return it.
I'll add a log for the unmarshal error
kodiakhq bot
pushed a commit
that referenced
this pull request
Dec 27, 2022
Related to cloudquery/cloudquery#5152. This fixes an issue when if the channel gets closed (for example there's an error during write), we report the `EOF` error instead of the original error. Scenario from the issue: 1. Users runs `sync --no-migrate` on a non initialized database 2. [`Write`](https://github.com/cloudquery/plugin-sdk/blob/ab7ca972e0b187a7dfb66132a03f07479cd29bb7/internal/servers/destinations.go#L84) fails on the first table write, returning an error from this function: https://github.com/cloudquery/plugin-sdk/blob/ab7ca972e0b187a7dfb66132a03f07479cd29bb7/internal/servers/destinations.go#L65, causing the channel to close 3. The next iteration of `for resource := range resources` starts, receiving an `EOF` error since the channel is closed, and `EOF` error is reported. > This is only a patch. I think we should separate application level errors (e.g. write) from protocol/communication level errors. I'll open a separate issue for that **Also, [another related fix coming](#461 ---
Member
Author
|
Closed by #529 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Related to cloudquery/cloudquery#5152 and #460
The error returned from the error group
Wait()function is the error reported by the Go routine, not the error of theWait()functionUse the following steps to ensure your PR is ready to be reviewed
go fmtto format your code 🖊golangci-lint run🚨 (install golangci-lint here)