GH-1026: Support KafkaHeaders.GROUP_ID #1030
Conversation
| * @return true to provide the header. | ||
| * @since 2.3 | ||
| */ | ||
| String exposeGroupId() default "false"; |
There was a problem hiding this comment.
I wonder why would one doesn't want to map this property to header?..
Why don't we do that unconditionally like for many other Kafka records properties?
There was a problem hiding this comment.
Doh - good point 😄
...ng-kafka/src/main/java/org/springframework/kafka/listener/KafkaMessageListenerContainer.java
Show resolved
Hide resolved
| protected final List<SimplePatternBasedHeaderMatcher> matchers = new ArrayList<>(NEVER_MAPPED); // NOSONAR | ||
| protected final Log LOGGER = LogFactory.getLog(getClass()); // NOSONAR | ||
|
|
||
| private static final List<HeaderMatcher> NEVER_MAPPED = Collections.singletonList( |
There was a problem hiding this comment.
Doesn't look like this constant is used any where.
Can't we just make it inline in the matchers bellow?
| protected final Log LOGGER = LogFactory.getLog(getClass()); // NOSONAR | ||
|
|
||
| private static final List<HeaderMatcher> NEVER_MAPPED = Collections.singletonList( | ||
| new NeverMatchHeaderMatcher(Arrays.stream(new String[] { |
There was a problem hiding this comment.
I would move toSet() into the NeverMatchHeaderMatcher ctor having its argument as a varargs variant to even avoid an explicit array here.
| * A matcher for headers. | ||
| * @since 2.3 | ||
| */ | ||
| protected interface HeaderMatcher { |
There was a problem hiding this comment.
So, the idea to let have any custom matcher only if we extend the whole class.
No way to reuse default one, but insert custom matchers.
Thanks
There was a problem hiding this comment.
I simply realized that having a pattern matcher for header names without a wildcard is much less efficient than looking up the header in a set.
The interface is so I can have 2 different types of matcher.
There was a problem hiding this comment.
But my point that it is protected and therefore not available if we don't extend.
The implementation is fine though, only concern is about convenience of end-users.
| * @param value the value. | ||
| * @param consumer the consumer. | ||
| * @return this. | ||
| * @since 5.2 |
There was a problem hiding this comment.
I think all these @since on methods are just wrong for this new class in this project.
There was a problem hiding this comment.
Doh - copy/paste.
Resolves spring-projects#1026 Also improve `DefaultKafkaHeaderMapper` with a `NeverMatchHeaderMatcher`.
spring-projects#1030 (comment) We removed the property from the annotation during PR review, but failed to correct the docs.
#1030 (comment) We removed the property from the annotation during PR review, but failed to correct the docs.
spring-projects#1030 (comment) We removed the property from the annotation during PR review, but failed to correct the docs.
2 participants
Resolves #1026
Also improve
DefaultKafkaHeaderMapperwith aNeverMatchHeaderMatcher.