fix: Don't hide errors in destination server#529
Conversation
erezrokah
left a comment
There was a problem hiding this comment.
Thanks for the PR @yevgenypats, added a few comments. Maybe we should sync on this to ensure I'm not missing anything with our error handling logic.
Also I'm not sure how this closes https://github.com/cloudquery/plugin-sdk/pull/463/files as it doesn't reduce the nesting.
Additionally #487 proposes a way to report multiple errors instead of using x and y failed
internal/servers/destinations.go
Outdated
| if err := eg.Wait(); err != nil { | ||
| s.Logger.Error().Err(err).Msg("got error. failed to wait for plugin") | ||
| if wgErr := eg.Wait(); wgErr != nil { | ||
| return status.Errorf(codes.Internal, "failed to receive msg: %v and wait for plugin: ", err, wgErr) |
There was a problem hiding this comment.
I think it's not accurate to report failed to wait for plugin.
The error returned from the wait group is whatever returned from the wait group Go Routine, i.e. whatever returned from s.Plugin.Write(ctx, tables, sourceName, syncTime, resources), so if eg.Wait() != nil it means the write failed, not that the wait failed.
Please let me know if that makes sense
internal/servers/destinations.go
Outdated
| close(resources) | ||
| if err := eg.Wait(); err != nil { | ||
| s.Logger.Error().Err(err).Msg("failed to wait") | ||
| return status.Errorf(codes.Internal, "Context done: %v and failed to wait for plugin: %v",ctx.Err(), err) |
There was a problem hiding this comment.
I'm not sure we should report the context error if the write failed. If the write fails the context error is always context cancelled so it has no value. See https://github.com/cloudquery/plugin-sdk/pull/460/files#diff-1df6a94b537b58b0f652697d4a670466f1f889553d4a55b654e4a818ea16a02fR113 and #460 (comment)
| return status.Errorf(codes.Internal, "Context done: %v and failed to wait for plugin: %v",ctx.Err(), err) | ||
| } | ||
| return ctx.Err() | ||
| return status.Errorf(codes.Internal, "Context done: %v", ctx.Err()) |
There was a problem hiding this comment.
See https://github.com/cloudquery/plugin-sdk/pull/486/files for how to convert context errors to gRPC status codes
43aa9ad to
36141fe
Compare
Co-authored-by: Erez Rokah <erezrokah@users.noreply.github.com>
⏱️ Benchmark resultsComparing with 5590845
|
🤖 I have created a release *beep* *boop* --- ## [1.14.0](v1.13.1...v1.14.0) (2022-12-27) ### Features * Add basic periodic metric INFO logger ([#496](#496)) ([8d1d32e](8d1d32e)) ### Bug Fixes * **destinations:** Stop writing resources when channel is closed ([#460](#460)) ([5590845](5590845)) * Don't hide errors in destination server ([#529](#529)) ([d91f94f](d91f94f)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Closes https://github.com/cloudquery/plugin-sdk/pull/512/files
Closes https://github.com/cloudquery/plugin-sdk/pull/463/files