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

Ensure that KafkaInterceptor @Component is in context #394

Open
alexandrepa opened this issue Oct 31, 2024 · 10 comments
Open

Ensure that KafkaInterceptor @Component is in context #394

alexandrepa opened this issue Oct 31, 2024 · 10 comments

Comments

@alexandrepa
Copy link
Collaborator

alexandrepa commented Oct 31, 2024

Using the tzatziki-kafka module, we have seen some users complaining about instability and struggling to test their kafka topics. It was due to the fact that KafkaInterceptor was not in the classpath. Tzatziki could not add some aspectj around kafka consumer.
We already added some instructions in the README to avoid that.

Second thing we can do is to check that the component is in the spring context. If it's not the case we could stops the test and log a message.

The purpose of this issue is to discuss how can we assure the KafkaInterceptor is initialized and also how do we want to address it to the user.

To check if KafkaInterceptor is initialized, maybe the best way is to @Autowired (optional) it in KafkaSteps and check in @Before method if its not null.

@jak78dkt
Copy link

jak78dkt commented Oct 31, 2024

Hey. As I said in previous discussions, I'm a strong supporter of this proposal. However, we have to consider the backward compatibility. Some people might not be happy that a Tzatziki upgrade make their test fail because they need time for undoing all their fixes in their Gherkin files they had to do to stabilize their tests. This is why I would suggest to provide an option to disable this check to give people time to migrate. In that case, the check could log an error instead of failing.

@alexandrepa
Copy link
Collaborator Author

I agree with you, seems a good way to operate.
By default, the tests will failed if there is no KafkaInterceptor, and the failure should mention the issue and the possibility to disable the failing of the test with a property.

@jak78dkt
Copy link

Exactly

@brian-mulier-p
Copy link

brian-mulier-p commented Oct 31, 2024

Hello ! Maybe this is worth giving a try ?

We do this by adding the name of the class in the standard file resources/META-INF/spring/org.springframework.boot.autoconfigure.AutoConfiguration.imports:

com.baeldung.autoconfiguration.MySQLAutoconfiguration

Adding the interceptor as an auto-import candidate should work but I might be wrong and maybe the mechanism comes too late for this case.

@alexandrepa
Copy link
Collaborator Author

I just did a test and it seems to be working 😄 thanks !

The only issue is how do we handle the users that are currently wrongly using the kafka lib in their test. Adding this could lead to failing test in their projects.

Maybe creating a major version with a warrant int the README. And add the possibility to disable the KafkaInterceptor.

@brian-mulier-p
Copy link

I'm not sure why it would fail those and in any case I think it's better in every aspect to auto-inject, otherwise you just don't take this dependency as the whole Kafka module is based-off the interceptor.
So IMO just make the change without caring about that (and if some users complain we can find a solution and maybe add a switch but pretty sure it's not needed)

@jak78dkt
Copy link

jak78dkt commented Nov 12, 2024

I'm not sure why it would fail

Some projets have adjusted/hacked their expected values in their test just make their tests repeatable, instead of fixing the tests using KafkaInterceptor. For these projects, enabling KafkaInterceptor will bring their test back to a normal behaviour, making their hacks fail. They need time to migrate their legacy tests by removing their hacks

I have changed my mind about the need for a switch: these users can stick to the previous version of Tzatziki while migrating, and upgrade to the latest Tzatziki version when their migration is complete.

@jak78dkt
Copy link

Hello ! Maybe this is worth giving a try ?

Thanks @brian-mulier-p that's a great idea!

@brian-mulier-p
Copy link

brian-mulier-p commented Dec 9, 2024

idk, did anyone give a try to do it so we can close this issue?

@alexandrepa
Copy link
Collaborator Author

I tested it and it worked yes thanks. Will make a PR when I have some times and link this issue to it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants