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.

Sign up for GitHub

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

Jump to bottom

fix: pubsub ack must rely on protocol.IsAck #1064

Merged
merged 6 commits into from
Jul 18, 2024
Merged

fix: pubsub ack must rely on protocol.IsAck #1064

merged 6 commits into from
Jul 18, 2024

Conversation

chapurlatn
Copy link
Contributor

@chapurlatn chapurlatn commented Jun 10, 2024 •
edited
Loading

PUB/SUB Message acking is missing protocol.Result support.
For example, stan seems to properly handle it: https://github.com/cloudevents/sdk-go/blob/main/protocol/stan/v2/message.go#L66.

This PR fixes the issue with related tests.

Old cases supported:

Function result Acked/Nacked Finish result
nil ACKED nil
any error NACKED the original error

New behaviour:

Function result Acked/Nacked Finish result
nil ACKED nil
any error except cloudevents.Result NACKED the original error
cloudevents.Result (param acked) ACKED nil
cloudevents.Result (param nacked) NACKED the protocol.Result

@chapurlatn chapurlatn requested a review from a team as a code owner June 10, 2024 12:27
@chapurlatn chapurlatn mentioned this pull request Jun 10, 2024
Open
@chapurlatn
Copy link
Contributor Author

Note: I've found an unclosed issue about that so I mentionned the PR is there: #834

embano1
embano1 reviewed Jun 15, 2024
View reviewed changes
Copy link
Member

@embano1 embano1 left a comment

Choose a reason for hiding this comment

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

Can you please also take a look at action item (2) here and update the doc string?

#834 (comment)

protocol/pubsub/v2/message.go Outdated Show resolved Hide resolved
protocol/pubsub/v2/message_test.go Outdated Show resolved Hide resolved
@chapurlatn

This comment was marked as outdated.

Sign in to view
@chapurlatn chapurlatn requested a review from embano1 June 18, 2024 10:21
@chapurlatn chapurlatn force-pushed the patch-1 branch 5 times, most recently from 975bf93 to 923d810 Compare June 20, 2024 08:24
embano1
embano1 reviewed Jun 22, 2024
View reviewed changes
v2/client/client.go Outdated Show resolved Hide resolved
@chapurlatn chapurlatn requested a review from embano1 June 24, 2024 09:28
embano1
embano1 reviewed Jun 29, 2024
View reviewed changes
v2/client/client.go Outdated Show resolved Hide resolved
chapurlatn
chapurlatn commented Jul 4, 2024
View reviewed changes
v2/client/client.go Outdated Show resolved Hide resolved
@chapurlatn chapurlatn requested a review from embano1 July 4, 2024 09:42
@chapurlatn
Copy link
Contributor Author

Hi @embano1,
The fork has been rebased from cloudevents/sdk-go main branch.
I'm on vacation during the next 2 weeks. I'll take a look anyway if you ask for changes.
Given previous comments, all has been taken into account apart the error propagation refactoring (but as I've suggested in comments, It may/should be handled in a separated PR).

embano1
embano1 reviewed Jul 16, 2024
View reviewed changes
Copy link
Member

@embano1 embano1 left a comment

Choose a reason for hiding this comment

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

Just a minor nit on the comment and +1 on keeping the existing Finish error handling behavior.

@duglin
Copy link
Contributor

duglin commented Jul 17, 2024

@chapurlatn wanna rebase and address Michael's comment?

@chapurlatn
fix: pubsub ack must rely on protocol.IsAck
b826898 
Signed-off-by: Nicolas Chapurlat <nc@coorganix.com>
@chapurlatn
doc(client): add information about the use of Receipt in StartReceiver
2e9d548 
Signed-off-by: Nicolas Chapurlat <nc@coorganix.com>
@chapurlatn
fix(pubsub): keep returning err event if acked is enabled
72dc26d 
Signed-off-by: Nicolas Chapurlat <nc@coorganix.com>
@chapurlatn
doc(client): simple error result for StartReceiver function is not acked
0acbebf 
Signed-off-by: Nicolas Chapurlat <nc@coorganix.com>
@chapurlatn
doc(client): better StartReceiver fn error doc
378a031 
Signed-off-by: Nicolas Chapurlat <nc@coorganix.com>
@chapurlatn
doc(client): keep each receiver func signatures in doc
b94c458 
Signed-off-by: Nicolas Chapurlat <nc@coorganix.com>
@chapurlatn
Copy link
Contributor Author

@embano1 @duglin Rebased, comments fixed as suggested, CI is green. Seems good to me now.

@chapurlatn chapurlatn requested a review from embano1 July 17, 2024 21:21
duglin
duglin approved these changes Jul 17, 2024
View reviewed changes
Copy link
Contributor

@duglin duglin left a comment

Choose a reason for hiding this comment

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

LGTM
@embano1 for the final review/merge

@embano1
Copy link
Member

embano1 commented Jul 18, 2024

Thank you!

@embano1 embano1 merged commit 5f0cc73 into cloudevents:main Jul 18, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@duglin duglin duglin approved these changes

@embano1 embano1 Awaiting requested review from embano1

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@chapurlatn @duglin @embano1