[fix][fn] Honour negativeAckRedeliveryDelayMs in the Python function runtime - #26413
Open
david-streamlio wants to merge 1 commit into
Open
[fix][fn] Honour negativeAckRedeliveryDelayMs in the Python function runtime#26413david-streamlio wants to merge 1 commit into
david-streamlio wants to merge 1 commit into
Conversation
…runtime The Python runtime negatively acknowledges on failure but never configured the redelivery delay, so the client default of 60 seconds applied regardless of what SourceSpec.negativeAckRedeliveryDelayMs carried. A function configured for fast retry, or for a long back-off from a struggling downstream, silently got neither. Add get_negative_ack_args() and splat its result into all three subscribe() call sites. Two details drive the shape: - The field is a proto3 scalar with no presence, so an unset value reads as 0. Only a positive value is forwarded, leaving the client default in place otherwise - the same guard JavaInstanceRunnable applies. Sending 0 through would mean immediate redelivery rather than the default. - The argument is omitted rather than passed as None. subscribe() validates it with _check_type(int, ...) and not _check_type_or_none, so None would raise for every function that does not configure it, unlike the neighbouring unacked_messages_timeout_ms which does accept None. Returning a dict to splat rather than a value keeps that omission at one site, since the first call site passes explicit keywords while the other two build a consumer_args dict. Fixes apache#26411 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
13 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #26411
Master Issue: #26412
Motivation
SourceSpec.negativeAckRedeliveryDelayMssets how long the broker waits before redelivering a negatively acknowledged message. The Python runtime negatively acknowledges on failure:but never configured the delay, so
subscribe()'s default of60000applied regardless of what the function was created with. A function configured for fast retry, or for a long back-off from a struggling downstream, silently got one minute either way.The Java runtime applies it. The Go runtime has the same gap, tracked at #26409.
Modifications
Add
PythonInstance.get_negative_ack_args()and splat its result into all threesubscribe()call sites — thetopicsToSerDeClassNameloop and both branches of theinputSpecsloop.Two details worth a reviewer's attention, both of which shaped the API rather than being incidental:
Unset reads as zero. The field is a proto3 scalar with no presence, so a function that never set it arrives with
0. Only a positive value is forwarded, leaving the client default in place otherwise — the same guardJavaInstanceRunnableapplies (if (sourceSpec.getNegativeAckRedeliveryDelayMs() > 0)). Forwarding0would mean immediate redelivery rather than the 60s default, which is not what an unconfigured function should get.The argument is omitted, not passed as
None.subscribe()validates this one with_check_type(int, negative_ack_redelivery_delay_ms, ...)rather than_check_type_or_none— unlike the neighbouringunacked_messages_timeout_ms, which the existing code does passNoneto. PassingNonehere would raise for every function that does not configure the field. Returning a dict to splat keeps that decision in one place, since the first call site passes explicit keywords while the other two build aconsumer_argsdict.The helper is a method rather than inline code so it is directly testable, matching
get_dead_letter_policy()added in #26400.Verifying this change
This change added tests and can be verified as follows:
test_python_instance.py(TestNegativeAckRedeliveryDelay): a positive delay is forwarded; an unset field is omitted; an explicit0is omitted; and the return value is a dict carrying exactly the keywordsubscribe()expects, since it is consumed via**and.update().pulsar-functions/instance/src/scripts/run_python_instance_tests.sh.get_negative_ack_args()return{}unconditionally fails 2 of them.Does this pull request potentially affect one of the following parts:
A function that does not set
negativeAckRedeliveryDelayMsis unaffected: the argument is omitted and the client default applies exactly as before.Documentation
doc-requireddoc-not-neededdocdoc-completeThe configuration option is already documented; this makes the Python runtime honour it.