-
Notifications
You must be signed in to change notification settings - Fork 26
feat: Support chunks in resource resolvers #2287
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
729634a to
105f97e
Compare
| type RowResolver func(ctx context.Context, meta ClientMeta, resource *Resource) error | ||
|
|
||
| type RowsChunkResolver struct { | ||
| ChunkSize int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need a way of ensuring ChunkSize is always less than 100 as that is what BatchSender uses
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or make BatchSender batch limit configurable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't matter:
- If you pass a slice on the channel
BatchSendersends the slice as is - If you pass individual items and whatever batch
BatchSendersends is smaller thanChunkSize, you'll have a single chunk with all the data
bbernays
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall I really like this, I do think that we should mark this as experimental so that we can break the function signature if necessary as we roll this out to our official plugins and make sure we are not missing any corner cases
|
Looks legit; I didn't have @bbernays context so I would have approved it. Let's wait for that and LGTM. |
|
I agree w/ Ben on this, we should defo mark it as experimental so we can switch signatures if needed. |
|
Made the feature experimental by adding a comment, see b3241e0#diff-abecd8d935ca8952a3554e15fa6989972dd0cea6de3410b6541931883b0d6d84R28 not sure if that's what you meant. I also added tests b3241e0#diff-2a41f52c87ab17afdba2deeea17e1182371ee8b6a1e1e8f4fcd19450c3adaeb8R192 for sending both single items (uses batch sender) and a slice to the channel (skips batch sender). I had to refactor our tests since:
|
|
@bbernays are you satisfied with my replies? Any follow up I should do here? |
Yes thank you, all good to ship 🚀 |
🤖 I have created a release *beep* *boop* --- ## [4.92.0](v4.91.0...v4.92.0) (2025-11-06) ### Features * Support chunks in resource resolvers ([#2287](#2287)) ([087ef9a](087ef9a)) ### Bug Fixes * **deps:** Update aws-sdk-go-v2 monorepo ([#2316](#2316)) ([828c4c2](828c4c2)) * **deps:** Update golang.org/x/exp digest to a4bb9ff ([#2315](#2315)) ([4bdaf94](4bdaf94)) * **deps:** Update module github.com/cloudquery/codegen to v0.3.33 ([#2321](#2321)) ([8080660](8080660)) * **deps:** Update module github.com/samber/lo to v1.52.0 ([#2318](#2318)) ([6e4c424](6e4c424)) * **deps:** Update module golang.org/x/oauth2 to v0.32.0 ([#2319](#2319)) ([48e0e01](48e0e01)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Summary
Fixes cloudquery/cloudquery#14601
Still untested, opening for referenceTested this in https://github.com/cloudquery/cloudquery-private/pull/9520 (internal link)
Use the following steps to ensure your PR is ready to be reviewed
go fmtto format your code 🖊golangci-lint run🚨 (install golangci-lint here)