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

Remove deprecated code for Micronaut Framework 5 and document breaking changes. #988

Draft
wants to merge 1 commit into
base: 6.0.x
Choose a base branch
from

Conversation

wetted
Copy link
Contributor

@wetted wetted commented Mar 15, 2024

closes #970

@wetted wetted added the type: breaking Introduces a breaking change label Mar 15, 2024
@wetted wetted self-assigned this Mar 15, 2024
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@wetted
Copy link
Contributor Author

wetted commented Mar 15, 2024

ping @dstepanov

io.micronaut.configuration.kafka.annotation.ErrorStrategyValue.NONE is still in the code. It was added as a deprecated enum on #441 as part of a fix for #372.

I didn't remove this yet because it's still used for code in the module and it's not clear to me how to deal with it. Please advise, or contribute as appropriate. This PR is meant to remove all deprecated code for Micronaut Framework 5.

@sdelamo sdelamo changed the base branch from 5.4.x to 6.0.x April 11, 2024 11:44
@wetted wetted removed their assignment Aug 8, 2024
@cwebbtw
Copy link

cwebbtw commented Dec 11, 2024

ping @dstepanov

io.micronaut.configuration.kafka.annotation.ErrorStrategyValue.NONE is still in the code. It was added as a deprecated enum on #441 as part of a fix for #372.

I didn't remove this yet because it's still used for code in the module and it's not clear to me how to deal with it. Please advise, or contribute as appropriate. This PR is meant to remove all deprecated code for Micronaut Framework 5.

Apologies for the late reply on this - for some context, I was one of the original contributors of the error strategies. We built this into micronaut as we were fixing #372 however there is still some lacking behaviour around handling batches (the current issue exists for batches today I believe).

One of the things that we discussed was not breaking existing code; take this below for example:

    @Override
    public void handle(final KafkaListenerException exception) {
        exception.getConsumerRecord().ifPresent(consumerRecord -> {
            TopicPartition topicPartition = new TopicPartition(consumerRecord.topic(), consumerRecord.partition());
            exception.getKafkaConsumer().seek(topicPartition, consumerRecord.offset());
        });
    }

This would be required if the error strategy is set to NONE to ensure that #372 isn't an issue for losing records in a poll. By setting RETRY_ON_ERROR this code that I showed is fundamentally defunct but it would change the behaviour of the kafka listener, as such, it was deprecated and the default behaviour should really be not to lose data (however this is probably not a decision I'm empowered to make on this project) since the other strategies will either skip over the problematic message. There is currently no dead letter queue error strategy unfortunately.

Also at some point I do hope to contribute to error strategies for batching, but I believe there's quite a few things that aren't working quite right for batch at the moment, e.g. redelivery not redelivering messages when batch=true and redelivery=true. I should probably raise an issue for this too but overall, this part in the documentation really does need to be recognised:

image

I'll ping on discord and see whether I can support this over my break from work during the festive season.

I hope this helps with the removal of deprecated code!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: breaking Introduces a breaking change
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

Del @Deprecated Kafka
3 participants