build: update to spring boot 4.1 - #863
Conversation
There was a problem hiding this comment.
Pull request overview
Upgrades Kafdrop to Spring Boot 4.1 and migrates affected web, Jackson, Actuator, testing, and Kafka APIs.
Changes:
- Replaces Undertow with Tomcat and adds TRACK filtering.
- Migrates Spring Boot 4.1 package and configuration APIs.
- Updates Kafka consumer-group listing API.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
pom.xml |
Updates Spring Boot, web server, Springdoc, and test dependencies. |
src/main/java/kafdrop/Kafdrop.java |
Replaces Undertow configuration with a TRACK filter. |
src/main/java/kafdrop/config/HealthCheckConfiguration.java |
Adapts health reporting to descriptor APIs. |
src/main/java/kafdrop/config/ObjectMapperConfig.java |
Migrates to Jackson 3 JsonMapper. |
src/main/java/kafdrop/controller/BasicErrorController.java |
Updates Spring Boot error API imports. |
src/main/java/kafdrop/service/KafkaHighLevelAdminClient.java |
Migrates Kafka group listing APIs. |
src/main/resources/application.yml |
Updates Jackson enum configuration. |
src/test/java/kafdrop/KafdropTest.java |
Migrates TestRestTemplate configuration. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| final Collection<GroupListing> groupListing; | ||
| try { | ||
| groupListing = adminClient.listConsumerGroups().valid().get(); | ||
| groupListing = adminClient.listGroups().valid().get(); |
There was a problem hiding this comment.
The STREAMS group type not working is actually what prompted my PR.
The call to listConsumerGroupOffsets happily accepts and returns the correct info for CLASSIC, CONSUMER, and STREAMS group types. If a SHARE group is passed in, it'll just ignore it.
There will need to be a little bit of refactoring to get share group information displayed.
There was a problem hiding this comment.
I've been reading a bit more about this and tend to agree with Copilot. It seems like listConsumerGroupOffsets will throw an exception when it receives something other than a consumer group.
So, I'd say there are two options:
- Rename this method into
listGroups, retain the filter behavior you just implemented (list all groups) and update callers of the renamed method to accept all types of groups. - Retain the name and the filtering, though by calling
listGroups(org.apache.kafka.clients.admin.ListGroupsOptions.forConsumerGroups())now.
There was a problem hiding this comment.
@Bert-R I've filtered out share groups. I don't think they'd be useful to anything calling this method because of how different they are.
I did leave streams groups in the filter. In the admin client, listStreamsGroupOffsets just calls the public listConsumerGroupOffsets method. The PR where the streams group was introduced notes that all the dedicated streams group methods are equivalent to the consumer group methods.
There is only 1 caller of this method in Kafdrop and it's just to get group offsets. I have this running against several Confluent Cloud brokers and can confirm it works correctly on classic, consumer, and streams group types. If we really want to filter for just consumer groups in this method, I can add another commit to call the streams group methods separately and merge the results.
|
@c4181 Thanks! This contribution is highly appreciated. It was on my list of things to do since long, but I couldn't find the time. I've asked Copilot for a review. See the review comments. Besides that, our linter complains about a line that is too long: Can you have a look? |
|
@c4181 Chris, Gihub has started rolling out a change, where PR merging is disallowed for unsigned commits. To ensure that that might not be the case here,
That should help shorten the PR cycle across all your PRs in github. |
feat: filter out share groups style: shorten line length fix: autowire JsonMapper.Builder
Update to Spring Boot 4.1