Skip to content

Conversation

@CallumNZ
Copy link
Contributor

Changes proposed in this pull request:

  • Add methods for SQS batch requests

Production Changes

The following production changes are required to deploy these changes:

  • None

Review

Check the box that applies to this code review. If necessary please seek help with adding a checklist guide for the reviewer.
When assigning the code review please consider the expertise needed to review the changes.

  • This is a content (documentation, web page etc) only change.
  • This is a minor change (meta data, bug fix, improve test coverage etc).
  • This is a larger change (new feature, significant refactoring etc). Please use the code review guidelines to add a checklist below to guide the code reviewer.

Code Review Guide

Insert check list here if needed.

@CallumNZ CallumNZ requested a review from a team March 19, 2025 00:56
Copy link
Contributor

@wilsonjord wilsonjord left a comment

Choose a reason for hiding this comment

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

I suspect it would be more complicated, but since []string is being passed to SendBatchN, perhaps instead of returning a slice of failed bodies, a slice of failed indexes could done? Returning body copies seems inefficient.

Comment on lines 261 to 268
type SendBatchError struct {
err error
info []SendBatchErrorInfo
}
type SendBatchErrorInfo struct {
entry types.BatchResultErrorEntry
body string
}
Copy link
Contributor

Choose a reason for hiding this comment

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

issue: error handling not quite as clean as it could be; seems to me that I need to check the Info field for empty, before knowing how to handle the error, which seems a little backwards. I also don't know why the fields aren't simply exported. Would you consider wrapping the general err, and separating it out from "partial message failure"? Something like:

var SendBatchPartialError = errors.New("partial message failure")

type SendBatchError struct {
    Err error
    Info []SendBatchResultErrorEntry
}

func (e *SendBatchError) Unwrap() error {
    return e.Err
}

void main() {
    err := sqs.SendBatch(payload)
    if errors.Is(err, SendBatchPartialError) {
        // deal with err.Info
    } else if err != nil {
        // general error, I can should be able to access wrapped error if desired
    }
}

aws/sqs/sqs.go Outdated
for i := range bodies {
// Check if any single message is too big
if len(bodies[i]) > maxSize {
sendBatch(i)
Copy link
Contributor

Choose a reason for hiding this comment

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

question: why this if we know message is too big?

@CallumNZ CallumNZ force-pushed the batchMethods branch 2 times, most recently from b9c89a3 to ea86180 Compare April 7, 2025 04:36
@CallumNZ CallumNZ requested a review from wilsonjord April 7, 2025 04:45
@CallumNZ CallumNZ force-pushed the batchMethods branch 2 times, most recently from 3495361 to 2935362 Compare April 8, 2025 01:10
@CallumNZ
Copy link
Contributor Author

CallumNZ commented Apr 8, 2025

@wilsonjord an example of using it: https://github.com/GeoNet/tilde/blob/b4cd55a4a5dd930bcfe78cc6034de4a27ff7788c/cmd/tilde-router/sqs-batcher.go#L59

The current idea is:

If there's an HTTP error, all elements of batch are added to Info.
If there's a partial error, just the failed elements of batch are added to Info.

So, if there's any sort of error, the user will be looping over Info to see which elements failed.

@CallumNZ CallumNZ force-pushed the batchMethods branch 2 times, most recently from f19f503 to 5a5f648 Compare April 11, 2025 00:32
@CallumNZ CallumNZ force-pushed the batchMethods branch 2 times, most recently from 71d7857 to c9f0d5a Compare April 11, 2025 04:21
Copy link
Contributor

@bpeng bpeng 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

Includes adding a test for this method
BREAKING CHANGE: now returns the number of API calls to SendBatch made
Changes the underlying receiveMethod function to return all messages rather than just the first
@sue-h-gns sue-h-gns merged commit d71b97b into main Apr 14, 2025
7 checks passed
@CallumNZ CallumNZ deleted the batchMethods branch April 17, 2025 04:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants