[fix][fn] Honour deadLetterTopic and maxMessageRetries in the Python function runtime - #26400
[fix][fn] Honour deadLetterTopic and maxMessageRetries in the Python function runtime#26400david-streamlio wants to merge 1 commit into
Conversation
…runtime FunctionConfig accepts maxMessageRetries and deadLetterTopic, both are carried into the instance as FunctionDetails.retryDetails, and the Java runtime applies them. The Python runtime ignored them entirely: nothing in python_instance.py referenced retryDetails, so a function created with --dead-letter-topic was accepted, reported back faithfully by functions get, and then routed nothing to the DLQ at runtime. Build a ConsumerDeadLetterPolicy from retryDetails in a new get_dead_letter_policy() and pass it to all three subscribe() call sites. The rules mirror the Java runtime, which guards on hasRetryDetails() in JavaInstanceRunnable and applies the policy in PulsarSource, setting the dead letter topic only when it is non-empty so the client can derive its "<topic>-<subscription>-DLQ" default. Two cases cannot mirror Java exactly, and both warn rather than failing the instance or silently doing nothing: - Java accepts maxMessageRetries >= 0, but the Python client's ConsumerDeadLetterPolicy rejects a redelivery count below 1, so zero cannot be expressed. Attaching no policy is the only option; a warning names the dead letter topic that will not receive messages. - A dead letter policy only takes effect on Shared and KeyShared subscriptions. retainOrdering and EFFECTIVELY_ONCE both select Failover, where the policy would be silently ineffective, so that combination warns too. Silently ineffective configuration is the bug this fixes, and it should not be reintroduced by the fix. Fixes apache#26397 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
A gap in this PR's reasoning that I found while auditing windowing support, and that a reviewer should weigh before merging. This PR makes the Python runtime honour // FunctionConfigUtils.doPythonChecks()
if (functionConfig.getMaxMessageRetries() != null && functionConfig.getMaxMessageRetries() >= 0) {
throw new IllegalArgumentException("Message retries not yet supported in python");
}So on master, That changes what this PR is worth on its own:
Options, and I do not have a strong view on which is right:
I would lean toward 1, since it keeps the runtime change reviewable on its own merits and makes the enabling change an explicit, visible decision rather than a side effect. Happy to open the follow-up either way. The same relationship exists for the Go runtime: |
Fixes #26397
Motivation
FunctionConfigacceptsmaxMessageRetriesanddeadLetterTopic, both are carried into the instance asFunctionDetails.retryDetails(Function.protoL58-61, L91), and the Java runtime applies them. The Python runtime ignored them entirely —grep -i "dead_letter\|retryDetails" python_instance.pyreturned nothing on master.The failure mode is silent, which is the damaging part. This is accepted without warning:
functions getreports the configuration back faithfully, and at runtime nothing is ever routed to the DLQ.Modifications
Add
get_dead_letter_policy()toPythonInstanceand pass its result to all threesubscribe()call sites inrun()— thetopicsToSerDeClassNameloop and both branches of theinputSpecsloop.The rules follow the Java runtime:
HasField("retryDetails"), matchingJavaInstanceRunnable'shasRetryDetails()check.HasFieldis already the idiom in this file (used forreceiverQueueSize).PulsarSource, so the client derives its<topic>-<subscription>-DLQdefault rather than receiving an empty name.Two cases cannot mirror Java exactly. Both warn rather than failing the instance or silently doing nothing:
maxMessageRetries == 0.PulsarSourceaccepts>= 0, but the Python client'sConsumerDeadLetterPolicyraisesValueErrorfor a redelivery count below 1 (pulsar/__init__.pyL761-762). Attaching no policy is the only available behaviour; the warning names the dead letter topic that will not receive messages. Raising here would take down a function that the Java runtime would have started.SharedandKeyShared.retainOrderingandEFFECTIVELY_ONCEboth selectFailover, where the policy would be silently ineffective — the same class of bug as this issue — so that combination warns as well.I did not change the Java-side
>= 0behaviour or the client's>= 1constraint; reconciling them is a larger discussion than this fix.Verifying this change
This change added tests and can be verified as follows:
test_python_instance.py(TestDeadLetterPolicy), covering: noretryDetails→ no policy; a policy built fromretryDetails; an empty dead letter topic deferring to the client default;maxMessageRetriesof 0 and of -1 attaching no policy rather than raising;KeySharedreceiving a policy;FailoverandExclusivenot.pulsar-functions/instance/src/scripts/run_python_instance_tests.sh.get_dead_letter_policy()returnNoneunconditionally fails 3 of them.Does this pull request potentially affect one of the following parts:
A function that does not set
retryDetailsis unaffected:get_dead_letter_policy()returnsNoneanddead_letter_policy=Noneis what the client already defaults to.Documentation
doc-requiredThe Pulsar Functions documentation states that
--dead-letter-topicand--max-message-retriesare supported without qualifying which runtimes honour them. #6084 was closed in 2020 by a docs update saying Python does not support this; with this change that note needs revisiting. I have not made the apache/pulsar-site change — flagging it so it is not lost, and happy to open it once the behaviour here is settled in review.