feat: Add local backend for storing cursor state by hermanschaaf · Pull Request #569 · cloudquery/plugin-sdk

hermanschaaf · 2023-01-03T21:10:14Z

A state store allows us to store cursors for #413. This first iteration introduces the interface and implements it for a local JSON storage backend. After some back and forth I decided to call it backend, similar to Terraform. Alternatives considered were state (confusing because it's not a single state) and storage (confusing because it's similar to destination).

Since the interface is a generic key-value store, we lose the ability to know whether a table is incremental or not, and which columns are used as cursor keys if it is. I'm proposing that we add some fields to the Table definition to be able to at least document these aspects, but it's not enforced.

See cloudquery/cloudquery#6336 for an example implementation using this new functionality.

github-actions · 2023-01-04T12:32:05Z

⏱️ Benchmark results

DefaultConcurrencyDFS-2 resources/s: 11,498
DefaultConcurrencyRoundRobin-2 resources/s: 11,786
Glob-2 ns/op: 205
TablesWithChildrenDFS-2 resources/s: 26,451
TablesWithChildrenRoundRobin-2 resources/s: 25,701
TablesWithRateLimitingDFS-2 resources/s: 28.26
TablesWithRateLimitingRoundRobin-2 resources/s: 815.8
BufferedScanner-2 ns/op: 12.38
LogReader-2 ns/op: 38.51

internal/backends/local/local.go

schema/column.go

yevgenypats

Looks really good. Don't have a lot of comments apart from some nits.

I do worry a bit on the global lock and Im curious to run some benchmarks with global and non global locks. If there is no difference then a global lock will be simple for sure if there is maybe we can change the api only to lock per table per client.

yevgenypats · 2023-01-04T18:32:01Z

codegen/templates/column.go.tpl

    {{- if .Options.PrimaryKey}}
      PrimaryKey: true,
    {{- end }}
+    {{- if .Options.IncrementalKey}}


Do we need this in codegen given we are deprecating this?

I think we may as well keep it here until the codegen gets removed (and then this entire file will get removed), don't see any harm in it

yevgenypats · 2023-01-04T18:34:43Z

plugins/source/plugin.go

 )

-type NewExecutionClientFunc func(context.Context, zerolog.Logger, specs.Source) (schema.ClientMeta, error)
+type NewExecutionClientFunc func(context.Context, zerolog.Logger, specs.Source, backend.Backend) (schema.ClientMeta, error)


Let's introduce an optional struct here so we can have a way of both adding required parameters that will break the API and once that are not required like this one (i.e not all plugins have to implement/use cursor)

Looking into this now

yevgenypats · 2023-01-04T18:38:45Z

internal/backends/local/local.go

+
+func (l *Local) Set(_ context.Context, table, key, value string) error {
+	l.tablesLock.Lock()
+	defer l.tablesLock.Unlock()


I think if you pre-alloc the tables you can avoid locking the whole map and just locking on a per table.

Maybe I'm misunderstanding, but I think we can't pre-alloc tables, because we don't know which tables will use this before Set is called. And if no tables use it, we don't want to write any file when the backend gets closed.

erezrokah

Looks good, but maybe we should consider making the API more restrictive

erezrokah · 2023-01-05T08:21:44Z

backend/backend.go

+
+import "context"
+
+type Backend interface {


Should we make these table methods? So the API will be t.SetCursor(value string)?

That would make it more restrictive, and avoid potential collisions. Also what's the reason we need a key? Can a single table have more than 1 key?

Yeah, a single table can have more than 1 key. For example, if the table is multiplexed on account or project, we may need to pass in different IDs for each of those accounts/projects to resume from the last position. Generally I think only very simple cases will have a single key, most cases will have many.

I like the table method idea! It might be hard to do in practice though. Right now resolvers don't have access to their own table, and even if they did, the table object wouldn't have a backend attached to it. I think we'll just have to accept that this will be the first pass, and we can iterate on the interface as we see how it's used in practice.

Yeah, a single table can have more than 1 key. For example, if the table is multiplexed on account or project, we may need to pass in different IDs for each of those accounts/projects to resume from the last position. Generally I think only very simple cases will have a single key, most cases will have many.

Multiplexed clients have an ID() method (I think tables can access it). Would that make a good key? I think that would make it easier to trace which cursor belongs to which client/table.

I think we'll just have to accept that this will be the first pass

Good point. If we see the API is used in an inconsistent way we can change it (unless we want it that way 😄 )

Yup, I haven't changed it to be a method on Table (which might be hard), but I have updated the Backend interface to be like this:

Set(ctx context.Context, table, clientID, value string) error

So basically we're relying on the client id to give uniqueness (which should be true if you use multiplexers correctly)

hermanschaaf · 2023-01-06T17:14:09Z

@yevgenypats Addressed all the comments and went through another iteration, this should be good for another look now.

hermanschaaf · 2023-01-06T17:33:03Z

internal/backends/local/local.go

+	l.tables[table][clientID] = value
+	if prev != value {
+		// only flush if the value changed
+		return l.flushTable(table, l.tables[table])


For now we are flushing on every Set call (if the value changed), but if we can ensure that Close always gets called before shutdown then we won't have to do that.

yevgenypats

Looks great! I might have some suggestions on improving the implementation or performance but I think The external API looks good so it should easy to adjust as we go.

Prob for next feature / improvement around that area I'd like to work on it to get more hands-on experience with that feature to see if I've any additional ideas around that.

🤖 I have created a release *beep* *boop* --- ## [1.24.0](v1.23.0...v1.24.0) (2023-01-09) ### Features * Add local backend for storing cursor state ([#569](#569)) ([3b07885](3b07885)) * Remove codegen ([#589](#589)) ([1c5943a](1c5943a)) ### Bug Fixes * **destinations:** Log correct size of batch ([#588](#588)) ([9cebafe](9cebafe)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

This adds a Hacker News source plugin that is able to download all items from the Hacker News API. It can be useful, but is mainly meant to be for instructional purposes, as this is the first example of a plugin making use of the state backend for incremental syncing being introduced in cloudquery/plugin-sdk#569 (this PR depends on that one).

Add backend for cursor state

872a189

github-actions bot added the feat label Jan 3, 2023

hermanschaaf added 4 commits January 4, 2023 08:45

Add lock

66c4ea0

Fix linting

4bc00f5

Fix linting

03075a6

Close backends in test

3012edc

hermanschaaf mentioned this pull request Jan 4, 2023

feat: Hacker News source plugin cloudquery/cloudquery#6336

Merged

hermanschaaf added 2 commits January 4, 2023 12:28

Add documentation options

42ae70b

Refactor

c9fde32

hermanschaaf added 3 commits January 4, 2023 12:33

Lint

eeddc7c

Add templating for IncrementalKey

29edf3a

Merge branch 'main' into incremental-syncing

94947d3

hermanschaaf marked this pull request as ready for review January 4, 2023 13:46

hermanschaaf requested a review from yevgenypats as a code owner January 4, 2023 13:46

github-actions bot added feat and removed feat labels Jan 4, 2023

hermanschaaf added 2 commits January 4, 2023 15:11

Merge branch 'main' into incremental-syncing

ab244d7

gofmt

0f54e78

disq reviewed Jan 4, 2023

View reviewed changes

internal/backends/local/local.go Outdated Show resolved Hide resolved

disq reviewed Jan 4, 2023

View reviewed changes

schema/column.go Outdated Show resolved Hide resolved

disq approved these changes Jan 4, 2023

View reviewed changes

hermanschaaf added 2 commits January 4, 2023 17:02

Remove unused field

d6f1e0f

Simplify error wrapping

020e75d

yevgenypats suggested changes Jan 4, 2023

View reviewed changes

erezrokah approved these changes Jan 5, 2023

View reviewed changes

hermanschaaf mentioned this pull request Jan 6, 2023

feat(alicloud): Add alicloud source plugin to sync bss and oss resources cloudquery/cloudquery#6418

Merged

hermanschaaf added 4 commits January 6, 2023 16:56

Add client to backend interface

b699aee

Use client id instead of key

a8ba259

Address review comments

65fc0c6

Merge branch 'main' into incremental-syncing

4539af7

hermanschaaf requested a review from yevgenypats January 6, 2023 17:13

hermanschaaf commented Jan 6, 2023

View reviewed changes

Make public

cc2bd95

yevgenypats approved these changes Jan 9, 2023

View reviewed changes

hermanschaaf added the automerge label Jan 9, 2023

Merge branch 'main' into incremental-syncing

5dc01fb

kodiakhq bot merged commit 3b07885 into main Jan 9, 2023

kodiakhq bot deleted the incremental-syncing branch January 9, 2023 13:32

cq-bot mentioned this pull request Jan 9, 2023

chore(main): Release v1.24.0 #586

Merged

hermanschaaf mentioned this pull request Jan 9, 2023

feature: Support cursor-based syncing #413

Closed

Conversation

hermanschaaf commented Jan 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jan 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⏱️ Benchmark results

Uh oh!

Uh oh!

Uh oh!

yevgenypats left a comment

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!

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!

erezrokah left a comment

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!

erezrokah Jan 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hermanschaaf commented Jan 6, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yevgenypats left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hermanschaaf commented Jan 3, 2023 •

edited

Loading

github-actions bot commented Jan 4, 2023 •

edited

Loading

erezrokah Jan 9, 2023 •

edited

Loading