Skip to content

fix: Initialise clients only once#473

Merged
kodiakhq[bot] merged 4 commits intomainfrom
fix/run_multiplex_ones
Dec 8, 2022
Merged

fix: Initialise clients only once#473
kodiakhq[bot] merged 4 commits intomainfrom
fix/run_multiplex_ones

Conversation

@yevgenypats
Copy link
Copy Markdown
Contributor

This fix a panic that was caused if plugins misbehave and return difference results for difference runs with the same config.

@github-actions github-actions bot added the fix label Dec 7, 2022
@yevgenypats
Copy link
Copy Markdown
Contributor Author

Still testing this

Copy link
Copy Markdown
Member

@erezrokah erezrokah left a comment

Choose a reason for hiding this comment

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

The fix looks good, but I won't kill the process because we can't send metrics. Maybe only log it?

@yevgenypats
Copy link
Copy Markdown
Contributor Author

The fix looks good, but I won't kill the process because we can't send metrics. Maybe only log it?

It's debugging :)

@yevgenypats
Copy link
Copy Markdown
Contributor Author

I think there were number of issues here and after fixing the initial issue with making sure Multiplex is called only once to combat with clients that return random clients on each call I was hit again by the flattening and global list. I'll look into refactoring this somehow next week

@github-actions
Copy link
Copy Markdown

github-actions bot commented Dec 7, 2022

⏱️ Benchmark results

Comparing with e34cb2c

  • DefaultConcurrency-2 resources/s: 11,286 ⬇️ 5.88% decrease vs. e34cb2c
  • Glob-2 ns/op: 161.6 ⬆️ 2.48% increase vs. e34cb2c
  • TablesWithChildrenDefaultConcurrency-2 resources/s: 30,577 ⬆️ 0.63% increase vs. e34cb2c
  • BufferedScanner-2 ns/op: 9.412 ⬇️ 7.73% decrease vs. e34cb2c
  • LogReader-2 ns/op: 30.79 (no change)

tables schema.Tables
// status sync metrics
metrics SourceMetrics
metrics *SourceMetrics
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this change to a pointer required for the fix?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think yes

@kodiakhq kodiakhq bot merged commit c88a521 into main Dec 8, 2022
@kodiakhq kodiakhq bot deleted the fix/run_multiplex_ones branch December 8, 2022 09:28
kodiakhq bot pushed a commit that referenced this pull request Dec 8, 2022
🤖 I have created a release *beep* *boop*
---


## [1.11.2](v1.11.1...v1.11.2) (2022-12-08)


### Bug Fixes

* Initialise clients only once ([#473](#473)) ([c88a521](c88a521))

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
kodiakhq bot pushed a commit to cloudquery/cloudquery that referenced this pull request Dec 8, 2022

#### Summary

An AWS sanity test that to catch cloudquery/cloudquery-issues#459 (internal issue). Related PR cloudquery/plugin-sdk#473.

Only runs on release PRs

<!--
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.

3 participants