Skip to content

Conversation

@erezrokah
Copy link
Member

Summary

This is more of hot fix/patch to #610.
#610 had an unwanted side effect on the migrate command, as now we init the plugin before running it.
initializing the plugin is expensive, requires credentials and does a whole lot of validations, for example:
https://github.com/cloudquery/cloudquery/blob/a06139efad9a2e8de14f186a223222c9d52ce58a/plugins/source/aws/client/client.go#L383
https://github.com/cloudquery/cloudquery/blob/a06139efad9a2e8de14f186a223222c9d52ce58a/plugins/source/azure/client/client.go#L124

Those seem unnecessary for tables migration (unless one uses dynamic tables).


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
Copy link

github-actions bot commented Jan 26, 2023

⏱️ Benchmark results

  • DefaultConcurrencyDFS-2 resources/s: 11,819
  • DefaultConcurrencyRoundRobin-2 resources/s: 12,379
  • Glob-2 ns/op: 244.5
  • TablesWithChildrenDFS-2 resources/s: 24,288
  • TablesWithChildrenRoundRobin-2 resources/s: 27,867
  • TablesWithRateLimitingDFS-2 resources/s: 28.4
  • TablesWithRateLimitingRoundRobin-2 resources/s: 814.8
  • BufferedScanner-2 ns/op: 12.29
  • LogReader-2 ns/op: 36.4

Copy link
Contributor

@yevgenypats yevgenypats left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Seems we need to add then the same workaround for GetDynamicTables. and use GetTablesForSpec in GetDynamicTables. Im also not a fan of this fix but I guess that's fine for now. basically it doesn make sense to authenticate to migrate in a way but yeah if dynamic_tables is nil then in theory this shouldn't be needed so that's the only workaround possible actually

@erezrokah
Copy link
Member Author

Seems we need to add then the same workaround for GetDynamicTables. and use GetTablesForSpec in GetDynamicTables

Can you explain this a bit? Not sure I got it, sessionTables is still initialized by Init even if you don't have dynamic tables

Copy link
Contributor

@yevgenypats yevgenypats left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. I think it's good then. I missed it in the GitHub UI, sorry.

@kodiakhq kodiakhq bot merged commit ece947f into cloudquery:main Jan 26, 2023
kodiakhq bot pushed a commit that referenced this pull request Jan 26, 2023
🤖 I have created a release *beep* *boop*
---


## [1.30.0](v1.29.0...v1.30.0) (2023-01-26)


### Features

* **destination:** Filter the duplicate primary keys prior to writing batch ([#629](#629)) ([505709e](505709e)), closes [#627](#627)


### Bug Fixes

* Ignore env variables in comments ([#625](#625)) ([08bace8](08bace8))
* Only call `newExecutionClient` if needed ([#630](#630)) ([ece947f](ece947f))

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants