Skip to content

Add jittered backoff and dead-letter fields to PulsarConfig - #5087

Open
mauriceyap wants to merge 2 commits into
dlq-ingester-2from
dlq-ingester-3
Open

Add jittered backoff and dead-letter fields to PulsarConfig#5087
mauriceyap wants to merge 2 commits into
dlq-ingester-2from
dlq-ingester-3

Conversation

@mauriceyap

Copy link
Copy Markdown
Collaborator

This extends PulsarConfig with MaxBackoffTime, BackoffRandomizationFactor, BackoffMultiplier, DeadLetterTopic, and DeadLetterMaxAttempts (validated gte=2), plus a Validate() method checking MaxBackoffTime >= BackoffTime. Note this is a pure config struct change for now - there is no consumer of these fields yet.

In a future PR, I will use cenkalti/backoff/v4 as a durect dependency to implement exponential backoff (it is currently an indirect).

@greptile-apps

greptile-apps Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

Extends Pulsar configuration for future retry and dead-letter handling.

  • Adds maximum backoff, jitter randomization, multiplier, dead-letter topic, and attempt-count fields.
  • Allows an omitted dead-letter attempt count while requiring configured values to be at least two.
  • Adds a cross-field backoff validation method and focused unit tests.

Confidence Score: 4/5

The PR is not yet safe to merge because normal startup validation still accepts a maximum backoff shorter than the initial backoff.

The omitted dead-letter attempt count is now accepted through the omitempty constraint, but the previously reported nested-validation issue remains: production parent validators call validate.Struct and never invoke PulsarConfig.Validate, while the new tests exercise that method only directly.

Files Needing Attention: internal/common/config/pulsar.go, internal/common/config/pulsar_test.go

Important Files Changed

Filename Overview
internal/common/config/pulsar.go Adds retry and dead-letter configuration fields, but the new cross-field validator remains disconnected from production configuration validation.
internal/common/config/pulsar_test.go Covers field-tag behavior and direct cross-field validation, but not validation through a production parent configuration.

Reviews (5): Last reviewed commit: "comment" | Re-trigger Greptile

Comment thread internal/common/config/pulsar.go Outdated
Comment thread internal/common/config/pulsar.go
@datadog-armadaproject

datadog-armadaproject Bot commented Aug 3, 2026

Copy link
Copy Markdown

Pipelines

⚠️ Warnings

🚦 1 Pipeline job failed

CI | All jobs succeeded   View in Datadog   GitHub Actions

See error Job did not run: blocked by a failed job in an earlier pipeline stage.

Useful? React with 👍 / 👎

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: fcfe5cd | Docs | Datadog PR Page | Give us feedback!

@mergify

mergify Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

  • Queue this pull request

@mauriceyap

Copy link
Copy Markdown
Collaborator Author

@greptile-apps we will use the Validate() function in #5092, part of the same stack as this PR.

@greptile-apps

greptile-apps Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Thanks for the context — that makes sense for a stacked PR workflow. The concern about Validate() being unused is resolved; happy to see it wired up in #5092.

nikola-jokic
nikola-jokic previously approved these changes Aug 5, 2026
This extends `PulsarConfig` with `MaxBackoffTime`, `BackoffRandomizationFactor`,
`BackoffMultiplier`, `DeadLetterTopic`, and `DeadLetterMaxAttempts` (validated `gte=2`), plus a
`Validate()` method checking `MaxBackoffTime >= BackoffTime`. Note this is a pure config struct change for now - there is no consumer of these fields yet.

In a future PR, I will use `cenkalti/backoff/v4` as a durect dependency to implement exponential backoff (it is currently an indirect).

Signed-off-by: Maurice Yap <mauriceyap@hotmail.co.uk>
Signed-off-by: Maurice Yap <mauriceyap@hotmail.co.uk>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants