Skip to content
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

[SQS] Ack messages when can not unmarshal payload #1364

Closed
mbruner opened this issue Sep 29, 2021 · 5 comments
Closed

[SQS] Ack messages when can not unmarshal payload #1364

mbruner opened this issue Sep 29, 2021 · 5 comments
Labels
enhancement New feature or request hacktoberfest

Comments

@mbruner
Copy link

mbruner commented Sep 29, 2021

When message payload received from SQS is not a valid JSON we get an error in logs but message is still acknowledged. That makes it impossible to forward it farther to DLQ.

log.Errorw("failed to marshal event data, will process next message...", zap.Error(err))
el.Metrics.EventProcessingFailed(el.GetEventSourceName(), el.GetEventName())
ack()
return

Also, SQS handling logic differs from how it's implemented for PubSub:

log.Errorw("failed to marshal the event data", zap.Error(err))
el.Metrics.EventProcessingFailed(el.GetEventSourceName(), el.GetEventName())
m.Nack()
return

@mbruner mbruner added the enhancement New feature or request label Sep 29, 2021
@whynowy
Copy link
Member

whynowy commented Sep 30, 2021

@mbruner - do you think adding a field indicating if dlq is configured and ack based on it works?

@whynowy
Copy link
Member

whynowy commented Sep 30, 2021

Also, do you have any suggestion to the logic for pub/sub?

@mbruner
Copy link
Author

mbruner commented Oct 11, 2021

do you think adding a field indicating if dlq is configured and ack based on it works?

@whynowy if it can be configurable - perfect!
I don't have any suggestion for pub/sub, only think that it should be consistent with sqs

@daniel-codefresh
Copy link
Member

@whynowy I can take this one, but what will be the preferable solution? to simply remove the ack to be consistent with PubSub logic? or to add a conditional ack based on a new dlq field on the spec like you suggested?

@daniel-codefresh
Copy link
Member

Hey @whynowy, heres the related PR: #1403
I ended up adding the dlq option you suggested to avoid breaking any existing behavior.

@whynowy whynowy closed this as completed Nov 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request hacktoberfest
Projects
None yet
Development

No branches or pull requests

3 participants