Skip to content
This repository has been archived by the owner on Jun 10, 2022. It is now read-only.

Auto create topics producer #215

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Sevavietl
Copy link

Thank you for the great library.

As it was mentioned in this issue it would be nice to be able to set topic creation option to be in line with auto.create.topics.enable broker option. According to docs the default option is true, so I added this to Kafka\Config.

Thank you in advance.

Copy link
Contributor

@lcobucci lcobucci left a comment

Choose a reason for hiding this comment

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

@Sevavietl thanks for your contribution!

Could you please add some functional tests to cover the auto creation? From my little knowledge about Kafka I don't think that these changes are enough to support that feature.


/**
* @doesNotPerformAssertions
* @phpcsSuppress SlevomatCodingStandard.TypeHints.TypeHintDeclaration.UselessDocComment
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't suppress CS violations

Copy link
Author

Choose a reason for hiding this comment

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

I took it from the test above

Copy link
Contributor

Choose a reason for hiding this comment

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

Ohh I get it, it's because we're using an old version of doctrine/coding-standard. Let's keep it for now and I created #216 to handle that. Thanks!

@Sevavietl
Copy link
Author

@lcobucci, I don't think functional tests needed, because my code doesn't manage actual Kafka settings (or saves the user from setting the wrong option). It simply ensures that you can acknowledge producer about auto topics creation, so that this check can pass (Kafka\Producer\RecordValidator):

if (! isset($topicList[$record['topic']])) {
      throw Exception\InvalidRecordInSet::nonExististingTopic($record['topic']);
}

Now it doesn't respects kafka broker option auto.create.topics.enable.

@lcobucci
Copy link
Contributor

@Sevavietl alright but we still need to ensure that the library works with that configuration enabled? Also how the library should behave when auto.create.topics.enable is not configured properly and someone bypasses the exception validation?

We have a huge issue in this library: we're doing many local tests but not adding them to the build and then we can't ensure that all edge cases will still work regardless of the changes we make.

@Sevavietl
Copy link
Author

Sevavietl commented May 23, 2018

@lcobucci, I agree that tests needed. If I understand correctly, configuring the Kafka itself is out of scope this library. So the only thing we can test in functional tests is the behavior: auto.create.topics.enable is disabled in Kafka, but the producer think it is enabled. To test such thing multiple Kafka containers need to be running (this option is set during process start). Such test will ensure that we handle this exceptional situation appropriately. I am not a docker nor Kafka aficionado (yet) so from the top of my head, it is hard to create such test infrastructure:)

From what I see in the docs of image used in tests:

to turn off automatic topic creation set KAFKA_AUTO_CREATE_TOPICS_ENABLE: 'false'

So for this particular test case there will be separate container (or even one container per each kafka version).

Does this looks ok to you? I would appretiate a direction, because as I said I am not a docker expert.

Thank you in advance.

@lcobucci
Copy link
Contributor

I am not a docker nor Kafka aficionado (yet) (...) Does this looks ok to you? I would appretiate a direction, because as I said I am not a docker expert.

@Sevavietl no worries, you're not alone 👍

I think that using a .env file would help us achieve this more easily (instead of creating many definitions of Kafka servers), these might be helpful: https://docs.docker.com/compose/env-file/ and https://docs.docker.com/compose/compose-file/#env_file. This file could also be used in the test runner execution, which would allow to know which scenario we're covering.

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

Successfully merging this pull request may close these issues.

2 participants