Skip to content

Add Kafka AutoConfiguration #5516

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed

Conversation

garyrussell
Copy link
Contributor

It needs more tests and docs.

But, before I do that, I need to get the approach validated/approved (there are a huge number of properties; I generated most of that code from the internal Kafka properties code).

I also need some guidance with regards to a starter - should that be part of this PR or a separate PR?

The auto-config itself is pretty straightforward; just a few beans to support a listener container factory and template.

Tested with a stand-alone boot app:

@SpringBootApplication
public class BootKafkaApplication {

    public static void main(String[] args) throws Exception {
        ConfigurableApplicationContext context = SpringApplication.run(BootKafkaApplication.class, args);
        @SuppressWarnings("unchecked")
        KafkaTemplate<String, String> template = context.getBean(KafkaTemplate.class);
        for (int i = 0; i < 10; i++) {
            template.convertAndSend("foo", 0, "bar", "baz" + i);
            Thread.sleep(1000);
        }
        context.close();
    }

    @Bean
    public Listener listener() {
        return new Listener();
    }

    public class Listener {

        @KafkaListener(topics="foo")
        public void listen(String in, @Header(KafkaHeaders.PARTITION_ID) int partition) {
            System.out.println("Received:" + in + " from partition: " + partition);
        }

    }

}

Needs a running Kafka with a topic foo and spring.kafka.consumer.group-id=foo in app.props.

@philwebb philwebb added for: team-attention An issue we'd like other members of the team to review and removed in progress labels Mar 28, 2016
@philwebb
Copy link
Member

@dsyer could you unlink spring-boot from waffle.io, the in-progress labels keep appearing.


@EnableKafka
@ConditionalOnMissingBean(name = KafkaListenerConfigUtils.KAFKA_LISTENER_ANNOTATION_PROCESSOR_BEAN_NAME)
protected static class EnableRabbitConfiguration {
Copy link
Member

Choose a reason for hiding this comment

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

Rabbit :-)

@wilkinsona
Copy link
Member

The diff for KafkaProperties is so large that GitHub won't let me comment on it. IMO, there are far too many. I'd much prefer that we support a commonly-used subset that's significantly smaller.

@garyrussell
Copy link
Contributor Author

Thanks - that was the feedback I was looking for - I didn't want to write a huge test case then throw it away. However, I am not sure how we will be able to determine what the subset should be. I guess we can just take a stab and refine it with user feedback.

@wilkinsona
Copy link
Member

However, I am not sure how we will be able to determine what the subset should be. I guess we can just take a stab and refine it with user feedback.

Perhaps we can offer an escape hatch in the form of a customiser interface that allows users to further configure the auto-configured stuff for cases where the properties don't cover their needs. KafkaConsumerCustomizer and KafkaProducerCustomizer would appear to be the main two in terms of the sheer number of properties.

@snicoll
Copy link
Member

snicoll commented Mar 29, 2016

What Andy said. We've done this recently for JMS and Rabbit btw, see b726974 and c4205d0

@garyrussell
Copy link
Contributor Author

Actually, they designate their props with an importance HIGH, MEDIUM and LOW - I figure I'll add just the HIGHs, one or two others and add a customizer for the rest - will rework.

@garyrussell garyrussell force-pushed the kafkaAutoConfig branch 2 times, most recently from c25aa62 to 70057d4 Compare March 29, 2016 20:50
@garyrussell
Copy link
Contributor Author

Reworked as discussed; squashed.

Still needs some more tests and docs but I want to see if this is closer to the mark.

@philwebb philwebb removed the for: team-attention An issue we'd like other members of the team to review label Mar 30, 2016
@philwebb
Copy link
Member

Looks much better now, do you have any idea how easy it will be to write tests for this?

@garyrussell
Copy link
Contributor Author

@philwebb If you mean integration tests with an embedded broker, theoretically it's possible - spring-kafka-test provides an embedded broker. However, when I added it to the CP, I got hundreds of test failures in other autoconfigs - perhaps because of some transitive deps pulled in by kafka itself, but it wasn't obvious to me what the problem was.

If you mean mocks to verify the autoconfig is set up correctly, that should be pretty easy.

If M2 was still tomorrow, I probably couldn't do it, but if the release calendar is correct I should be able to have tests in a day or so.

@wilkinsona
Copy link
Member

I got hundreds of test failures in other autoconfigs - perhaps because of some transitive deps pulled in by kafka itself, but it wasn't obvious to me what the problem was.

I'm interested in what those failures were. It might suggest that there's an incompatibility that we need to deal with.

If M2 was still tomorrow, I probably couldn't do it, but if the release calendar is correct I should be able to have tests in a day or so.

M2's scheduled for next week (6th April) now as we're waiting for Spring Framework 4.3.0.RC1

@garyrussell
Copy link
Contributor Author

@wilkinsona I figured it out - it was our old friend slf4j; excluding that from the test dependency solved it.

Produced some weird failures, though; which is what threw me and made me think it was something worse...

Failed tests: 
  FreeMarkerAutoConfigurationTests.nonExistentTemplateLocation 
Expected: (a string containing "Cannot find template location")
     but: a string containing "Cannot find template location" was ""
...

@wilkinsona
Copy link
Member

@garyrussell Thanks. We have some tests that capture System.out and System.err to make sure that the right things got logged. I would guess that the extra SLF4J dependencies (slf4j-nop, perhaps?), changed the output that was logged.

@garyrussell garyrussell changed the title [REVIEW ONLY] Add Kafka AutoConfiguration Add Kafka AutoConfiguration Mar 31, 2016
@garyrussell
Copy link
Contributor Author

Pushed tests for remaining properties and an integration test.

Removed 'REVIEW ONLY' from PR title.

@wilkinsona
Copy link
Member

@garyrussell Sorry, I have some bad news… Every field in KafkaProperties and its inner classes needs javadoc that will be used when generating the configuration metadata. We should probably be 100% sure that we'll merge this before you go through that pain.

@garyrussell
Copy link
Contributor Author

@wilkinsona It won't be a big deal to copy it from Kafka if/when I get a green light.

@philwebb
Copy link
Member

@garyrussell Don't forget that we don't support Javadoc tags (@link etc) in the meta-data file.

@garyrussell garyrussell force-pushed the kafkaAutoConfig branch 2 times, most recently from efa423a to ec65e32 Compare April 1, 2016 19:03
@garyrussell
Copy link
Contributor Author

Rebased, pushed property docs.

@philwebb philwebb added for: team-attention An issue we'd like other members of the team to review type: enhancement A general enhancement labels Apr 4, 2016
Change Common Props to Hyphenated (asciidoc)

Only Support HIGH and a Few Other Props
Also change filename properties to Resources.
@philwebb
Copy link
Member

@garyrussell What's the release plan for spring-kafka? When is GA due?

@garyrussell
Copy link
Contributor Author

@philwebb Early May - the RC will certainly be before boot's RC1.

@philwebb philwebb removed the for: team-attention An issue we'd like other members of the team to review label Apr 16, 2016
@garyrussell
Copy link
Contributor Author

@philwebb Let me know if there is some remaining reticence about merging this - we are about to put out the spring-kafka release candidate - if the auto config doesn't go into boot, we can move it to the project itself. TBH, it's not really clear to me how such decisions are made.

Cheers.

@garyrussell
Copy link
Contributor Author

I am going to close this - we just hit a blocker in spring-kafka and might not make an end of month GA.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants