-
Notifications
You must be signed in to change notification settings - Fork 13
feat: end to end wiring for per message nack #220
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
Changes from all commits
a517350
5906e28
07023ef
f008eba
d0db7fa
a711b57
b94e1ae
e9936dc
2e8d807
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| package io.numaproj.numaflow.shared; | ||
|
|
||
| import lombok.Builder; | ||
| import lombok.Getter; | ||
|
|
||
| /** | ||
| * NackOptions carries per-message redelivery options for a negative acknowledgement (nack). | ||
| * All fields are optional; a null value means unset. | ||
| */ | ||
| @Getter | ||
| @Builder(builderMethodName = "newBuilder") | ||
| public class NackOptions { | ||
| /** redelivery delay in milliseconds. */ | ||
| private final Long delay; | ||
| /** maximum number of redelivery attempts. */ | ||
| private final Integer maxDeliveries; | ||
| /** human-readable reason for the nack. */ | ||
| private final String reason; | ||
|
|
||
| /** Converts to the outgoing proto type, setting only the fields that are present. */ | ||
| public common.NackOptionsOuterClass.NackOptions toProto() { | ||
| common.NackOptionsOuterClass.NackOptions.Builder b = | ||
| common.NackOptionsOuterClass.NackOptions.newBuilder(); | ||
| if (delay != null) { | ||
| b.setDelay(delay); | ||
| } | ||
| if (maxDeliveries != null) { | ||
| b.setMaxDeliveries(maxDeliveries); | ||
| } | ||
| if (reason != null) { | ||
| b.setReason(reason); | ||
| } | ||
| return b.build(); | ||
| } | ||
|
|
||
| /** Converts from the incoming proto type. Returns null for null input. */ | ||
| public static NackOptions fromProto(common.NackOptionsOuterClass.NackOptions p) { | ||
| if (p == null) { | ||
| return null; | ||
| } | ||
|
Comment on lines
+21
to
+40
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. are these methods exposed to users?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, as they currently stand, they are exposed to the users.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think if I remember correctly we had this discussion when we wanted to add user/system metadata. These methods require to be public because the internal callers to these shared classes are in different packages.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wish java had access modifies similar to rust |
||
| NackOptionsBuilder b = NackOptions.newBuilder(); | ||
| if (p.hasDelay()) { | ||
| b.delay(p.getDelay()); | ||
| } | ||
| if (p.hasMaxDeliveries()) { | ||
| b.maxDeliveries(p.getMaxDeliveries()); | ||
| } | ||
| if (p.hasReason()) { | ||
| b.reason(p.getReason()); | ||
| } | ||
| return b.build(); | ||
| } | ||
| } | ||
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.
Just see if we can give an option for users to customise or add more options.
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.
Let me do this in separate PR, since we need this in both core and SDK