GH-3067: Draft of mapping multiple headers with same key

[poznachowski]

Draft of mapping iterable headers

[artembilan]

I'm OK with the solution.
Please, consider to run gradlew check locally, so you will see Checkstyle violations.
Thanks

[artembilan]

Please, also add your name to the @author list of the affected classes.
The whats-new.adoc deserves a new entry for this new feature.
And headers.adoc must be improved about this new behavior.

I also see that you did nothing for the DefaultKafkaHeaderMapper.
Probably the logic you have introduced can go into the AbstractKafkaHeaderMapper ?

[poznachowski]

Yes of course. This was just a starter to make sure I'm on the right track.

[sobychacko]

@poznachowski, Any updates on this? We have a release coming up next week. If you want this to be part of that release, we need to start further review on this PR sooner.

[poznachowski]

I should have something ready tomorrow.

[poznachowski]

I updated my MR with initial version (not polished) of DefaultHeaderKafkaMapper. It's more complicated than I thought.
The need of handling spring_json_header_types header makes it harder to extract common logic to the AbstractKafkaHeaderMapper.
I'm not really sure if my solution will hold. I see that RecordHeaders uses List internally, but is it enough to guarantee that Kafka ensures headers order?
Additionally, the change makes DeliveryHeaderTests fail. retry_topic-attempts header becomes now a list with 2 entries and getNonBlockingRetryDeliveryAttempt method fails. Haven't investigated it yet.

[sobychacko]

@poznachowski We moved the milestone to 3.2.0-RC1, which is the April release, so you have some time to polish things. Please let us know when it is ready for the actual review. The approach that you take in the PR is in the right direction (generally speaking). Could you polish and make sure that there are no checkstyle issues, etc.? Then we will do more formal reviews.

[poznachowski]

@sobychacko sure, should have it ready somewhere beginning next week.

[sobychacko]

@poznachowski Any updates on this PR?

[poznachowski]

I'd like to say that I have all ready, but I found some Spring specific header inconsistencies and struggling with picking the right approach.

retainAllRetryHeaderValues flag in DeadLetterPublishingRecovererFactory allows to have multiple headers on Kafka-side for given header types (especially RetryTopicHeaders.DEFAULT_HEADER_ATTEMPTS). When these were mapped onto Spring Message only the last one was always retained allowing to simply use @Header(name = RetryTopicHeaders.DEFAULT_HEADER_ATTEMPTS, required = false) Integer nonBlockingAttempts (as in DeliveryHeaderTests). As I was unware of usage of such headers, the current state of my MR allows to map these to a potential List on Spring side.
This has developed into another problem. Spring was still able to map List of values onto a single element with usage of CollectionToObjectConverter. The converter is taking first element thought. To overcome this and allow potential backwards compatibility I decided to reverse the list order while mapping.
Current solution is also inconsistent as I decided to keep only last values for headers such as: KafkaHeaders.DELIVERY_ATTEMPT, KafkaHeaders.LISTENER_INFO, KafkaUtils.KEY_DESERIALIZER_EXCEPTION_HEADER and KafkaUtils.VALUE_DESERIALIZER_EXCEPTION_HEADER as they where already specified in the DefaultKafkaHeaderMapper mapper. When I tried to allow multiple values for KafkaHeader.DELIVERY_ATTEMPT multiple test issues arose.

Having said that I'd suggest to keep a list of Spring 'technical' Kafka headers that should never be mapped into a collection on the Spring side (only last values should be kept). This would avoid unnecessary changes in the code (especially in the retrytopic package). If this plus reversing list order (so CollectionToObjectConverter can pick up the last value for backwards compatibility) sounds fine to you I can proceed with polishing and providing appropriate documentation.

[sobychacko]

@poznachowski Sorry for the delay in responding to you. The approach you mentioned for keeping the internal metadata type headers as only single-value item makes sense. As long as it doesn't bring any backward compatibility issues, it's ok to reverse the order. Please re-work the PR and when ready, remove the draft status, so that we can start a formal PR review. Thanks!

[artembilan]

Please, consider to not reformat Javadocs: no empty lines for methods.

[artembilan]

I'm not sure what you are doing here, but I think all the framework headers must be as a single value Kafka headers anyway. There are a lot of logic in the framework which relies on them.
We just must agree that we have to make a fix only for end-user headers which can be iterable.
Even if that JSON headers looks like a collection, it is really something what the framework rely on as as single header.
So, I don't expect too drastic changes in this area.
Thanks.

[sobychacko]

@poznachowski Are you going to continue working on this PR? We can also consider taking it on our end and start making additional changes on top of your PR. Please let us know the status.

[sobychacko]

@poznachowski, I just wanted to see if there is an update on this PR. If you are busy, we can take this PR and work on it on our end. Please let us know. Thank you.

[poznachowski]

@sobychacko Sorry for lack of response. I was quite unavailable, but back now. If you allow I will try to finalize the MR next week the latest.

[sobychacko]

@poznachowski, Any updates on this PR? We are still in the milestone stages of the 3.3.0 release, and if you update the PR, we can potentially include this feature in the RC1 timeline.