feat: Add CQ type system to support multiple destinations by yevgenypats · Pull Request #320 · cloudquery/plugin-sdk

yevgenypats · 2022-10-30T16:54:00Z

This add a type system to CloudQuery SDK.

This is mandatory to support multiple destinations.

Also, this found quite a few bugs where we were sending random stuff over the wire without any validation apart from maybe when we were hitting the database and then failing batches all together.

In CloudQuery type system I used heavily https://github.com/jackc/pgtype and kept the license and the copyright in it's own package cqtype.

This is a continue of this PR #298 where I split it into this one and #318

to go with cloudquery/plugin-sdk#320 and cloudquery/plugin-sdk#318

erezrokah

Added a few comments and will continue to review tomorrow 📖

clients/destination.go

clients/source.go

plugins/source.go

internal/servers/source.go

erezrokah · 2022-10-30T19:00:38Z

plugins/destination.go

+	eg := &errgroup.Group{}
+	// given most destination plugins writing in batch we are using a worker pool to write in parallel
+	// it might not generalize well and we might need to move it to each destination plugin implementation.
+	for i := 0; i < writeWorkers; i++ {


Since writeWorkers is always 1 maybe we can remove this for now?

yeah we can. I was playing with that, wasn't sure what it should be and how it generalize to most plugins.

plugins/source_test.go

plugins/destination.go

Co-authored-by: Erez Rokah <erezrokah@users.noreply.github.com>

plugins/destination_testing.go

Co-authored-by: Herman Schaaf <hermanschaaf@gmail.com>

cqtypes/convert.go

hermanschaaf · 2022-10-31T11:13:10Z

cqtypes/inet.go

+// Network address family is dependent on server socket.h value for AF_INET.
+// In practice, all platforms appear to have the same value. See
+// src/include/utils/inet.h for more information.
+const (
+// defaultAFInet  = 2
+// defaultAFInet6 = 3
+)


Looks like this constant block is now unused 🤔

yes. removed. it is used in pgx to encode to postgresql binary format but we don't need it as we just need the decode part and not from postgresql

hermanschaaf · 2022-10-31T11:25:12Z

internal/servers/destinations.go


+	eg := errgroup.Group{}
+	eg.Go(func() error {
+		return s.Plugin.Write(context.Background(), tables, sourceName, syncTime, resources)


Just trying to understand: why is this write now using context.Background() instead of msg.Context() ?

Good catch. I think just got confused in the context maze

internal/servers/destinations.go

plugins/destination_testing.go

schema/column.go

hermanschaaf · 2022-10-31T11:43:36Z

schema/resource.go

+		panic(columnName + " column not found")
+	}
+	if err := r.data[index].Set(value); err != nil {
+		panic(fmt.Errorf("failed to set column %s: %w", columnName, err))


Should this be a panic? It seems like if we set a column and it fails, we should return an error but otherwise leave it empty.

Yeah it should panic because it mean what the user provided is of incorrect value and defined the wrong column. We don't have a way of knowing about this (in sentry and in logs) other then panic as all other errors are classified as error. But I do think it's a panic in any case.

serve/source.go

Co-authored-by: Herman Schaaf <hermanschaaf@gmail.com>

clients/source_terminate_windows.go

clients/destination.go

plugins/source_testing.go

Co-authored-by: Herman Schaaf <hermanschaaf@gmail.com>

🤖 I have created a release *beep* *boop* --- ## [0.13.16](v0.13.15...v0.13.16) (2022-10-31) ### Features * Add CQ type system to support multiple destinations ([#320](#320)) ([d3b24a0](d3b24a0)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

to go with cloudquery/plugin-sdk#320 and cloudquery/plugin-sdk#318

This is instead of #3176 SDK PRs: cloudquery/plugin-sdk#318 cloudquery/plugin-sdk#320 Previous related CloudQuery PRs: #3286 Co-authored-by: Herman Schaaf <hermanschaaf@gmail.com> Co-authored-by: Erez Rokah <erezrokah@users.noreply.github.com>

This is instead of cloudquery/cloudquery#3176 SDK PRs: cloudquery/plugin-sdk#318 cloudquery/plugin-sdk#320 Previous related CloudQuery PRs: cloudquery/cloudquery#3286 Co-authored-by: Herman Schaaf <hermanschaaf@gmail.com> Co-authored-by: Erez Rokah <erezrokah@users.noreply.github.com>

feat: Add CQ type system to support multiple destinations

b2f08b0

github-actions bot added feat and removed feat labels Oct 30, 2022

yevgenypats requested review from erezrokah and hermanschaaf October 30, 2022 16:56

github-actions bot added feat and removed feat labels Oct 30, 2022

yevgenypats mentioned this pull request Oct 30, 2022

feat: Metrics and Type system #298

Closed

yevgenypats added a commit to cloudquery/cloudquery that referenced this pull request Oct 30, 2022

feat: Migrate cli, plugins and destinations to new type system

ada271d

to go with cloudquery/plugin-sdk#320 and cloudquery/plugin-sdk#318

yevgenypats mentioned this pull request Oct 30, 2022

feat: Migrate cli, plugins and destinations to new type system cloudquery/cloudquery#3323

Merged

erezrokah requested changes Oct 30, 2022

View reviewed changes

yevgenypats and others added 2 commits October 30, 2022 22:05

Update plugins/source.go

c17057c

Co-authored-by: Erez Rokah <erezrokah@users.noreply.github.com>

fix timestamp type

7d5d6d4

hermanschaaf mentioned this pull request Oct 31, 2022

fix: Silence error log when plugin exits #308

Closed

yevgenypats added 3 commits October 31, 2022 11:21

fix linter

52c03b8

fix graceful shutdown for windows

ef44050

add metrics to client back

bd1326f

hermanschaaf reviewed Oct 31, 2022

View reviewed changes

plugins/destination_testing.go Outdated Show resolved Hide resolved

Update plugins/destination_testing.go

7847ff0

Co-authored-by: Herman Schaaf <hermanschaaf@gmail.com>

hermanschaaf suggested changes Oct 31, 2022

View reviewed changes

yevgenypats and others added 3 commits October 31, 2022 14:47

add more tests to specreader

01be389

more bufixes and destination testing improvment

95ad28e

Update serve/source.go

049b88e

Co-authored-by: Herman Schaaf <hermanschaaf@gmail.com>

hermanschaaf reviewed Oct 31, 2022

View reviewed changes

clients/source_terminate_windows.go Show resolved Hide resolved

hermanschaaf reviewed Oct 31, 2022

View reviewed changes

clients/destination.go Outdated Show resolved Hide resolved

fix source plugin tests

6772f87

hermanschaaf reviewed Oct 31, 2022

View reviewed changes

plugins/source_testing.go Show resolved Hide resolved

yevgenypats and others added 2 commits October 31, 2022 17:16

Update clients/destination.go

35a5182

Co-authored-by: Herman Schaaf <hermanschaaf@gmail.com>

remove unused code

a173ae3

yevgenypats added 4 commits October 31, 2022 17:22

remove more unused code

b5cf16c

fix context

2403425

fix lint

49de841

fix tests

4456196

yevgenypats requested review from erezrokah and hermanschaaf October 31, 2022 15:34

hermanschaaf approved these changes Oct 31, 2022

View reviewed changes

fix minor logging stuff

015a63e

yevgenypats added the automerge label Oct 31, 2022

yevgenypats merged commit d3b24a0 into main Oct 31, 2022

yevgenypats deleted the feat/cq_type_system branch October 31, 2022 15:44

cq-bot mentioned this pull request Oct 31, 2022

chore(main): Release v0.13.16 #321

Merged

yevgenypats added a commit to cloudquery/cloudquery that referenced this pull request Oct 31, 2022

feat: Migrate cli, plugins and destinations to new type system

4839c76

to go with cloudquery/plugin-sdk#320 and cloudquery/plugin-sdk#318

yevgenypats added a commit to cloudquery/cloudquery that referenced this pull request Oct 31, 2022

feat: Migrate cli, plugins and destinations to new type system

6c9e08e

to go with cloudquery/plugin-sdk#320 and cloudquery/plugin-sdk#318

This was referenced Oct 31, 2022

Re-add https://github.com/cloudquery/cq-provider-sdk/pull/453 after cqv2 is released. cloudquery/cloudquery#1833

Closed

feat: Add CSV destination plugin cloudquery/cloudquery#3371

Closed

Conversation

yevgenypats commented Oct 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erezrokah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

yevgenypats commented Oct 30, 2022 •

edited

Loading