feat: Deterministic _cq_id #712

bbernays · 2023-02-24T20:07:04Z

Summary

When making cq_id deterministic we cannot use any of the existing resolver types because we cannot guarantee that the value has been fetched by that time so in this PR I call a new function after the resource has been fully resolved in resolveResource

Use the following steps to ensure your PR is ready to be reviewed

Read the contribution guidelines 🧑‍🎓
Run go fmt to format your code 🖊
Lint your changes via golangci-lint run 🚨 (install golangci-lint here)
Update or add tests 🧪
Ensure the status checks below are successful ✅

github-actions · 2023-02-24T20:12:34Z

⏱️ Benchmark results

Comparing with b2b8de9

DefaultConcurrencyDFS-2 resources/s: 11,617 ⬆️ 1.64% increase vs. b2b8de9
DefaultConcurrencyRoundRobin-2 resources/s: 12,485 ⬆️ 9.61% increase vs. b2b8de9
Glob-2 ns/op: 297.9 ⬆️ 35.35% increase vs. b2b8de9
TablesWithChildrenDFS-2 resources/s: 23,872 ⬇️ 26.73% decrease vs. b2b8de9
TablesWithChildrenRoundRobin-2 resources/s: 23,426 ⬇️ 28.16% decrease vs. b2b8de9
TablesWithRateLimitingDFS-2 resources/s: 28.23 ⬇️ 0.74% decrease vs. b2b8de9
TablesWithRateLimitingRoundRobin-2 resources/s: 836.5 ⬆️ 1.97% increase vs. b2b8de9
BufferedScanner-2 ns/op: 12.41 ⬆️ 24.26% increase vs. b2b8de9
LogReader-2 ns/op: 41.16 ⬆️ 25.44% increase vs. b2b8de9

schema/resource.go

candiduslynx

This will require a follow-up PR to overwrite _cq_id and _cq_parent_id where they've been skipped.

codecov-commenter · 2023-02-27T15:24:01Z

Codecov Report

Patch coverage: 47.05% and project coverage change: +1.69 🎉

Comparison is base (78530f8) 47.17% compared to head (52444d1) 48.87%.

📣 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     #712      +/-   ##
==========================================
+ Coverage   47.17%   48.87%   +1.69%     
==========================================
  Files          70       70              
  Lines        6849     6877      +28     
==========================================
+ Hits         3231     3361     +130     
+ Misses       3167     3054     -113     
- Partials      451      462      +11

Impacted Files	Coverage Δ
plugins/source/scheduler_dfs.go	`65.47% <0.00%> (-5.04%)`	⬇️
schema/meta.go	`7.14% <ø> (-2.86%)`	⬇️
specs/source.go	`57.14% <ø> (ø)`
schema/resource.go	`39.28% <72.72%> (+16.70%)`	⬆️
schema/convert.go	`35.29% <0.00%> (ø)`
schema/json.go	`45.16% <0.00%> (+1.61%)`	⬆️
schema/float8.go	`24.85% <0.00%> (+1.69%)`	⬆️
schema/int8.go	`25.30% <0.00%> (+1.80%)`	⬆️
schema/inet.go	`50.00% <0.00%> (+2.45%)`	⬆️
schema/int8_array.go	`26.77% <0.00%> (+2.51%)`	⬆️
... and 14 more

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.

yevgenypats · 2023-02-28T07:15:50Z

Looks good. Can we make it an opt-int option given we don't know the impact of this on compute at scale/real-world scenarios?

schema/resource.go

specs/source.go

candiduslynx

I'm OK with the changes. However, the problems with deterministic CQ ID for append mode in PG, MS SQL & other destinations should be addressed before this could be merged, hence requesting changes just to block the PR merge.

schema/resource.go

bbernays · 2023-03-01T11:46:25Z

@candiduslynx- I don't think we need to fix those issues prior to releasing this because the default behavior hasn't changed. Users have to opt in to change the behavior.

yevgenypats

Sorry I missed it in prior reviews but we shouldn't use here hashstructure library. We already have a type system in place so no need for reflection. Either adding Hash function for each of them using string and hashing this with io.writer and hasher otherwise we don't really control the hashing mechanism also hashstructure will be much slower.

…gin-sdk into cq-id-deterministic

schema/resource.go

yevgenypats

Looks good.

Few non-blocking comments:

bytes.Buffer (instead of append)
the current slack thread - do we want to have random uuid for things without pk in any case.

🤖 I have created a release *beep* *boop* --- ## [1.41.0](v1.40.0...v1.41.0) (2023-03-02) ### Features * Deterministic _cq_id ([#712](#712)) ([2e7ad2c](2e7ad2c)) * **multiplex:** Detect duplicated clients ([#723](#723)) ([dfb039d](dfb039d)) ### Bug Fixes * Cleanup code ([#710](#710)) ([963f03c](963f03c)) * **deps:** Update golang.org/x/exp digest to c95f2b4 ([#718](#718)) ([de52c10](de52c10)) * **deps:** Update google.golang.org/genproto digest to 9b19f0b ([#719](#719)) ([ecfddea](ecfddea)) * **deps:** Update module github.com/rivo/uniseg to v0.4.4 ([#720](#720)) ([0da69b6](0da69b6)) * **deps:** Update module github.com/stretchr/testify to v1.8.2 ([#721](#721)) ([19c0742](19c0742)) * **pk:** Skip filter for no PK ([#709](#709)) ([d0c2e26](d0c2e26)) * **types-json:** Disable HTML escaping during JSON marshalling ([#714](#714)) ([2f6f1d8](2f6f1d8)) * **types-timestamp:** Ensure timestamp is UTC ([#716](#716)) ([bb33629](bb33629)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

working

c3d9a1e

bbernays changed the title ~~Working Deterministic _cq_id~~ feat: Deterministic _cq_id Feb 24, 2023

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

bbernays marked this pull request as ready for review February 24, 2023 20:24

bbernays requested a review from yevgenypats as a code owner February 24, 2023 20:24

disq reviewed Feb 24, 2023

View reviewed changes

schema/resource.go Outdated Show resolved Hide resolved

erezrokah mentioned this pull request Feb 26, 2023

Connecting Relation Tables to Parent Table cloudquery/cloudquery#8415

Closed

disq approved these changes Feb 27, 2023

View reviewed changes

candiduslynx approved these changes Feb 27, 2023

View reviewed changes

Create resource_test.go

29a6e55

bbernays added 3 commits February 27, 2023 10:01

Update resource_test.go

3e020b8

Update resource_test.go

5a227d2

Update resource_test.go

158d253

bbernays added 5 commits February 28, 2023 10:04

Update resource.go

cb5ee03

Update source.go

5aaf2f0

Update resource_test.go

0b7cd54

Update scheduler_dfs.go

ba5bdb1

Update plugin_test.go

48241a1

erezrokah reviewed Feb 28, 2023

View reviewed changes

schema/resource.go Outdated Show resolved Hide resolved

bbernays added 3 commits February 28, 2023 10:10

Update resource.go

6ec4c7b

renamed

296cbfe

linting

948bfe0

erezrokah reviewed Feb 28, 2023

View reviewed changes

schema/resource.go Show resolved Hide resolved

erezrokah reviewed Feb 28, 2023

View reviewed changes

schema/resource.go Outdated Show resolved Hide resolved

erezrokah reviewed Feb 28, 2023

View reviewed changes

specs/source.go Outdated Show resolved Hide resolved

bbernays added 2 commits February 28, 2023 10:27

more naming

484a08c

Merge branch 'main' into cq-id-deterministic

12513af

candiduslynx suggested changes Mar 1, 2023

View reviewed changes

schema/resource.go Outdated Show resolved Hide resolved

yevgenypats suggested changes Mar 1, 2023

View reviewed changes

bbernays added 2 commits March 1, 2023 07:18

Remove hashstructure

d407d38

Merge branch 'cq-id-deterministic' of https://github.com/bbernays/plu…

67cb936

…gin-sdk into cq-id-deterministic

candiduslynx approved these changes Mar 1, 2023

View reviewed changes

schema/resource.go Outdated Show resolved Hide resolved

bbernays added 5 commits March 1, 2023 07:59

Update resource.go

52444d1

fix tests

432f00b

Merge branch 'main' into cq-id-deterministic

b4c697b

Update resource_test.go

e0d74b6

Update resource_test.go

f4651be

yevgenypats approved these changes Mar 1, 2023

View reviewed changes

Only hash PK fields

4328ddb

bbernays requested review from erezrokah, yevgenypats and hermanschaaf March 1, 2023 19:13

yevgenypats approved these changes Mar 1, 2023

View reviewed changes

bbernays added the automerge label Mar 1, 2023

kodiakhq bot merged commit 2e7ad2c into cloudquery:main Mar 1, 2023

cq-bot mentioned this pull request Mar 1, 2023

chore(main): Release v1.41.0 #711

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Deterministic _cq_id #712

feat: Deterministic _cq_id #712

bbernays commented Feb 24, 2023 •

edited

Loading

github-actions bot commented Feb 24, 2023 •

edited

Loading

candiduslynx left a comment

codecov-commenter commented Feb 27, 2023 •

edited

Loading

yevgenypats commented Feb 28, 2023

candiduslynx left a comment

bbernays commented Mar 1, 2023

yevgenypats left a comment

yevgenypats left a comment

feat: Deterministic _cq_id #712

feat: Deterministic _cq_id #712

Conversation

bbernays commented Feb 24, 2023 • edited Loading

Summary

github-actions bot commented Feb 24, 2023 • edited Loading

⏱️ Benchmark results

candiduslynx left a comment

Choose a reason for hiding this comment

codecov-commenter commented Feb 27, 2023 • edited Loading

Codecov Report

yevgenypats commented Feb 28, 2023

candiduslynx left a comment

Choose a reason for hiding this comment

bbernays commented Mar 1, 2023

yevgenypats left a comment

Choose a reason for hiding this comment

yevgenypats left a comment

Choose a reason for hiding this comment

bbernays commented Feb 24, 2023 •

edited

Loading

github-actions bot commented Feb 24, 2023 •

edited

Loading

codecov-commenter commented Feb 27, 2023 •

edited

Loading