feat: Improve migration detection APIs #688

yevgenypats · 2023-02-15T19:31:46Z

Migration detection APIs take 2.

This creates one API that returns all the changes instead of multiple growing APIs for different set of changes.
Improves Migration tests for different use-cases

This will come up also with a Postgres PR and documentation PR:

Basically each destination plugin would have the following table:

Migrations Logic

Change	Behaviour
Add Column	Auto Migrate/Drop Table
Add Column with Not Null	Auto Migrate/Drop Table
Remove Column	Auto Migrate/Drop Table
Remove Column with Not Null	Auto Migrate/Drop Table
Change Column	Auto Migrate/Drop Table

and we can easily expand it in the code, tests and documentations.

Example of usage in cloudquery/cloudquery#7819

github-actions · 2023-02-15T19:37:40Z

⏱️ Benchmark results

DefaultConcurrencyDFS-2 resources/s: 9,394
DefaultConcurrencyRoundRobin-2 resources/s: 12,447
Glob-2 ns/op: 255.1
TablesWithChildrenDFS-2 resources/s: 26,926
TablesWithChildrenRoundRobin-2 resources/s: 26,193
TablesWithRateLimitingDFS-2 resources/s: 28.38
TablesWithRateLimitingRoundRobin-2 resources/s: 817.2
BufferedScanner-2 ns/op: 11.73
LogReader-2 ns/op: 38.06

erezrokah

Added a few comments, I don't think any are blocking

erezrokah · 2023-02-16T10:01:52Z

schema/table.go

+
+// changed pks: drop table // table.IsPrimaryKeyEqual(other)
+
+func (t *Table) GetChanges(other *Table) []TableColumnChange {


Suggested change

func (t *Table) GetChanges(other *Table) []TableColumnChange {

func (new *Table) GetChanges(old *Table) []TableColumnChange {

Maybe this will make it easier to follow? Non blocking

new being a reserved word probably can't be used, but the old change is 👍

schema/table.go

internal/memdb/memdb_test.go

plugins/destination/plugin_testing_migrate.go

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

hermanschaaf · 2023-02-16T11:11:26Z

plugins/destination/plugin_testing.go

+// type MigrateResult int
+
+// const (
+// 	MigrateResultSuccess MigrateResult = iota
+// 	MigrateResultDropTable
+// )


Looks like this can be removed?

plugins/destination/plugin_testing.go

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

codecov-commenter · 2023-02-16T11:28:18Z

Codecov Report

Base: 47.19% // Head: 47.08% // Decreases project coverage by -0.12% ⚠️

Coverage data is based on head (8f4d92b) compared to base (1b52253).
Patch coverage: 54.16% of modified lines in pull request are covered.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #688      +/-   ##
==========================================
- Coverage   47.19%   47.08%   -0.12%     
==========================================
  Files          70       70              
  Lines        6831     6845      +14     
==========================================
- Hits         3224     3223       -1     
- Misses       3156     3171      +15     
  Partials      451      451

Impacted Files	Coverage Δ
schema/column.go	`50.00% <0.00%> (ø)`
schema/table.go	`42.85% <47.22%> (-4.49%)`	⬇️
internal/memdb/memdb.go	`85.23% <81.81%> (-0.48%)`	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

plugins/destination/plugin_testing_migrate.go

hermanschaaf · 2023-02-16T11:22:07Z

plugins/destination/plugin_testing.go

+type MigrateStrategy struct {
+	AddColumn           specs.MigrateMode
+	AddColumnNotNull    specs.MigrateMode
+	RemoveColumn        specs.MigrateMode
+	RemoveColumnNotNull specs.MigrateMode
+	ChangeColumn        specs.MigrateMode
+}


It might be just semantics, but I'm a bit confused by the options here 🤔

The table in the PR description has two options for each entry: Auto Migrate / Drop Table. I think it would be clearer if we used that terminology here, rather than MigrateMode = forced or MigrateMode = safe. forced/safe doesn't convey the same meaning to me drop/auto, but I think that is the intention of this configuration, right?

Yeah I had the same dilemma if to introduce another type, but then it would be quite annoying as we will have OverwriteForceStrategy OverwriteAutoMigrateStrategy and the same for append so I preferred for now just to do one for each mode and then we can understand if it is being dropped or not.

But I think that yes, we potentially will need to change it, I just didn't want to make it even more complex right now as I think we will need to learn a bit more about this API and usage and then do another iteration on the testing API at least.

Maybe a comment would help for now. Something like forced == drop table, safe == do not drop table

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

plugins/destination/plugin_testing_migrate.go

hermanschaaf

Nice, looks good 👍 If we want to split hairs I'd probably change the naming of the options a bit to make them clearer (IMO), and I spotted a few error messages we may want to fix, but other than that all good :)

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

🤖 I have created a release *beep* *boop* --- ## [1.38.0](v1.37.1...v1.38.0) (2023-02-16) ### Features * Improve migration detection APIs ([#688](#688)) ([dc3bedf](dc3bedf)) ### Bug Fixes * Better string methods for TableColumnChange ([#690](#690)) ([a0ec52c](a0ec52c)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

Blocked by cloudquery/plugin-sdk#668 This streamlines migrations, introduce force option and in general should speed up migrations substantially as we only do now one query instead of query per table. Blocked by another take in SDK - cloudquery/plugin-sdk#688

yevgenypats added 2 commits February 15, 2023 21:16

feat: Add better migration detection APIs

08b0f45

fmt

0943a5c

yevgenypats requested review from erezrokah and hermanschaaf February 15, 2023 19:31

github-actions bot added the feat label Feb 15, 2023

yevgenypats added 2 commits February 15, 2023 21:45

more fixes

0c21e45

more fixes

a3326de

github-actions bot added feat and removed feat labels Feb 15, 2023

yevgenypats mentioned this pull request Feb 15, 2023

feat(pg): Faster migrations cloudquery/cloudquery#7819

Merged

fix lint

99cc560

erezrokah approved these changes Feb 16, 2023

View reviewed changes

yevgenypats and others added 5 commits February 16, 2023 12:54

Update internal/memdb/memdb_test.go

f5e4cec

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

Update internal/memdb/memdb_test.go

5ee41b2

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

Update internal/memdb/memdb_test.go

f81a0be

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

Update internal/memdb/memdb_test.go

8ca2cd0

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

review fix

6ffffc8

hermanschaaf reviewed Feb 16, 2023

View reviewed changes

plugins/destination/plugin_testing.go Outdated Show resolved Hide resolved

yevgenypats and others added 2 commits February 16, 2023 13:23

fix review

d6ae733

Update plugins/destination/plugin_testing.go

8f4d92b

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

hermanschaaf reviewed Feb 16, 2023

View reviewed changes

Update plugins/destination/plugin_testing_migrate.go

7b779bd

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

hermanschaaf reviewed Feb 16, 2023

View reviewed changes

plugins/destination/plugin_testing_migrate.go Outdated Show resolved Hide resolved

hermanschaaf reviewed Feb 16, 2023

View reviewed changes

plugins/destination/plugin_testing_migrate.go Outdated Show resolved Hide resolved

hermanschaaf approved these changes Feb 16, 2023

View reviewed changes

yevgenypats and others added 2 commits February 16, 2023 13:38

Update plugins/destination/plugin_testing_migrate.go

5a0cc5a

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

Update plugins/destination/plugin_testing_migrate.go

57704b8

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

yevgenypats added the automerge label Feb 16, 2023

kodiakhq bot merged commit dc3bedf into main Feb 16, 2023

kodiakhq bot deleted the fix/migration_sdk branch February 16, 2023 11:42

cq-bot mentioned this pull request Feb 16, 2023

chore(main): Release v1.38.0 #689

Merged


		// changed pks: drop table // table.IsPrimaryKeyEqual(other)

		func (t Table) GetChanges(other Table) []TableColumnChange {

	func (t Table) GetChanges(other Table) []TableColumnChange {
	func (new Table) GetChanges(old Table) []TableColumnChange {

feat: Improve migration detection APIs #688

feat: Improve migration detection APIs #688

Uh oh!

Conversation

yevgenypats commented Feb 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

⏱️ Benchmark results

Uh oh!

erezrokah left a comment

Choose a reason for hiding this comment

Uh oh!

erezrokah Feb 16, 2023

Choose a reason for hiding this comment

Uh oh!

disq Feb 16, 2023

Choose a reason for hiding this comment

Uh oh!

yevgenypats Feb 16, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hermanschaaf Feb 16, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov-commenter commented Feb 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

hermanschaaf Feb 16, 2023

Choose a reason for hiding this comment

Uh oh!

yevgenypats Feb 16, 2023

Choose a reason for hiding this comment

Uh oh!

hermanschaaf Feb 16, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hermanschaaf left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

yevgenypats commented Feb 15, 2023 •

edited

Loading

github-actions bot commented Feb 15, 2023 •

edited

Loading

codecov-commenter commented Feb 16, 2023 •

edited

Loading