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.

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

Implement config for the backoff middleware #85

Merged
merged 2 commits into from
Dec 13, 2023

Conversation

rilder-almeida
Copy link
Contributor

Issue:

During an incident in which we experienced a message bottleneck that was being indefinitely reattempted, we realized that the backoff middleware was instantiating the default configurations of the retrier and not allowing these configurations to be passed as parameters.

Solution:

Add to the configuration structure of the pipelines configurations for the backoff middleware's retrier. To maintain backward compatibility, if these configurations are not defined, default configurations will be used. Additionally, if a Dead Letter Queue (DLQ) topic is defined in the config but the number of retries is set to infinite, a warning log indicating the risk of an infinite loop will be emitted during resource instantiation.

Problema:

Durante um incidente, onde tivemos um gargalo de mensagens que ficaram sendo retentadas infinitamente, percebemos que o middleware de backoff instanciava as configs padrões do retrier e não permitia que estas configs fossem passadas como parâmetro.

Solução:

Adiciona na estrutura de configuração do pipelineas configs para o retrier do backoff middleware. Para não haver nenhuma quebra de combatibilidade, caso estas configs não sejam definidas, configs padrões serão usadas. E se na presença de um tópico de DLQ definido nas config mas o numero de retentativas for infinito, um log de warnning sinalizando o risco de um loop infinito será emitido durante a instanciação dos recursos.

Links:

 - https://app.clickup.com/t/86a0j6ea8
Copy link
Member

@rjfonseca rjfonseca left a comment

Choose a reason for hiding this comment

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

LGTM

But I pointed out some concerns. Mainly that this new configuration could mislead the developers in thinking that there is a DLQ when in practice it will never be used. I would recommend that this particular case should raise an error instead of a warning.

pipeline/builder.go Outdated Show resolved Hide resolved
pipeline/builder.go Outdated Show resolved Hide resolved
Copy link
Member

@rjfonseca rjfonseca left a comment

Choose a reason for hiding this comment

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

LGTM

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@rilder-almeida rilder-almeida merged commit 59a8d52 into master Dec 13, 2023
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.

3 participants