feat: Add managed API for destination plugins#521
Conversation
⏱️ Benchmark resultsComparing with 1fe91cb
|
hermanschaaf
left a comment
There was a problem hiding this comment.
Mostly looks good - left a couple of questions / possible points for discussion
schema/timestamptz.go
Outdated
| return false | ||
| } | ||
|
|
||
| if dst.Time.Sub(value.Time) < 1*time.Second { |
There was a problem hiding this comment.
Oof, this might bite us later. Why is it necessary to round to 1 second?
There was a problem hiding this comment.
yeah. ok I think I work around that with rounding up our auto-generated data in the tests to seconds. I only hit this snag in csvs.
Co-authored-by: Herman Schaaf <hermanschaaf@gmail.com>
Co-authored-by: Herman Schaaf <hermanschaaf@gmail.com>
Co-authored-by: Herman Schaaf <hermanschaaf@gmail.com>
Co-authored-by: Herman Schaaf <hermanschaaf@gmail.com>
Co-authored-by: Herman Schaaf <hermanschaaf@gmail.com>
|
|
||
| type Option func(*client) | ||
|
|
||
| func WithErrOnWrite() Option { |
There was a problem hiding this comment.
Intended for testing, these options should have Test somewhere (either godoc or in the option name, like WithTestingErrOnWrite, WithTestingBlockingWrite)
There was a problem hiding this comment.
those are all under internal so it can't be exported anyway for outside use.
erezrokah
left a comment
There was a problem hiding this comment.
Looks good, but we might want to try and make the managed writer code easier to follow in future PRs.
Had two non blocking comments
| } | ||
| resources = make([][]interface{}, 0) | ||
| } | ||
| case done := <-flush: |
There was a problem hiding this comment.
What's the reason we need the flush? Won't closing ch have the same result?
There was a problem hiding this comment.
because write can be called multiple times so we need to wait for a flush from a specific worker and we can't close the channel as other worker might be still using this worker, so we just wanted to do a flush.
Co-authored-by: Herman Schaaf <hermanschaaf@gmail.com>
Co-authored-by: Herman Schaaf <hermanschaaf@gmail.com>
Co-authored-by: Herman Schaaf <hermanschaaf@gmail.com>
Co-authored-by: Herman Schaaf <hermanschaaf@gmail.com>
Co-authored-by: Erez Rokah <erezrokah@users.noreply.github.com>
Co-authored-by: Herman Schaaf <hermanschaaf@gmail.com>
Co-authored-by: Herman Schaaf <hermanschaaf@gmail.com>
Co-authored-by: Herman Schaaf <hermanschaaf@gmail.com>
Co-authored-by: Herman Schaaf <hermanschaaf@gmail.com>
Co-authored-by: Herman Schaaf <hermanschaaf@gmail.com>
🤖 I have created a release *beep* *boop* --- ## [1.13.0](v1.12.7...v1.13.0) (2022-12-21) ### Features * Add managed API for destination plugins ([#521](#521)) ([3df6129](3df6129)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
This should go after this is merged cloudquery/plugin-sdk#518 Blocked by cloudquery/plugin-sdk#521
This adds a managed API for destination plugins.
Closes #518 as it got destroyed with conflicts.
This should go with that: cloudquery/cloudquery#5805