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

kafka sandbox example #9762

Closed

Conversation

amre-abb
Copy link

Description: Add a simple sandbox example for kafka filter
Risk Level: Low
Testing: Built docs locally and verified the generated docs.
Docs Changes: Yes
Release Notes: Add a sandbox example for kafka filter.
[Optional Fixes #Issue]
[Optional Deprecated:]

@amre-abb amre-abb force-pushed the amre--add-kafka-sandbox-example branch 3 times, most recently from 88c39f8 to 1a5a448 Compare January 21, 2020 19:26
@lucperkins
Copy link
Contributor

@amre-abb Please provide a Developer Certificate of Origin

@mattklein123
Copy link
Member

This is awesome! @adamkotwasinski PTAL

@adamkotwasinski
Copy link
Contributor

LGTM (thank you!)

@amre-abb
Copy link
Author

Thanks @adamkotwasinski and @mattklein123 for a quick review. I will look at the CI/build failures and address @lucperkins comment soon.

P.S. @adamkotwasinski thanks a lot for writing a kafka filter! it is awesome!

@amre-abb amre-abb force-pushed the amre--add-kafka-sandbox-example branch 2 times, most recently from d621233 to c6dc4b8 Compare January 22, 2020 16:30
@mattklein123 mattklein123 self-assigned this Jan 22, 2020
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thank you so much for this! One small comment.

/wait

examples/kafka/envoy.yaml Show resolved Hide resolved
- filters:
- name: envoy.filters.network.kafka_broker
typed_config:
"@type": type.googleapis.com/envoy.config.filter.network.kafka_broker.v2alpha1.KafkaBroker
Copy link
Contributor

@moderation moderation Jan 22, 2020

Choose a reason for hiding this comment

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

I know this is jumping a bit outside of this PR for the sandbox but the actual Kafka documentation does not use the proper typed_config which it should. RST to be updated at https://github.com/envoyproxy/envoy/blob/master/docs/root/configuration/listeners/network_filters/kafka_broker_filter.rst. I can open a separate PR or ask @adamkotwasinski to add this to his Kafka version update PR at #9582

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 Thanks! I'll fix it there

Signed-off-by: amre_shakimov <[email protected]>
... and move kafka.rst file to a correct location

Signed-off-by: amre_shakimov <[email protected]>
Signed-off-by: amre_shakimov <[email protected]>
Signed-off-by: amre_shakimov <[email protected]>
Signed-off-by: amre_shakimov <[email protected]>
Signed-off-by: amre_shakimov <[email protected]>
@amre-abb amre-abb force-pushed the amre--add-kafka-sandbox-example branch from b8cebf2 to e7f19b0 Compare January 23, 2020 21:38
@mattklein123
Copy link
Member

@amre-abb friendly request to not force push, it breaks review history. Just merge master and add commits. Thanks!

@mattklein123
Copy link
Member

Very close now but you have a deprecated field to fix: https://dev.azure.com/cncf/envoy/_build/results?buildId=28392&view=logs&j=ec9f720e-f904-5f65-4ba4-f371a013344f&t=ee47c280-92b1-5c80-3a10-d3d23a58a062&l=2294

Thank you!

/wait


.. code-block:: console

$ docker run --rm -it --network envoymesh wurstmeister/kafka:2.11-2.0.0 /opt/kafka/bin/kafka-console-producer.sh --broker-list envoy:19092 --topic test_topic
Copy link
Contributor

@moderation moderation Jan 25, 2020

Choose a reason for hiding this comment

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

The Kafka filter currently supports Kafka version 2.3.1 in master - #9582). The Docker image versions you have here is 2.11 Scala / 2.0.0 Kafka which are about a year old. Should we setup the sandbox using the latest Kafka 2.3.1 container at wurstmeister/kafka:2.12-2.3.1 ?

Copy link
Contributor

@adamkotwasinski adamkotwasinski Jan 25, 2020

Choose a reason for hiding this comment

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

2.3.1 would be a safer bet, 2.4.0 is going to involve more C++ work than usual
(later on I can just grep for 2.3.1 in whole repo and make PRs for review by other folks so we don't get out of sync)

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree. I just tested the sandbox with 2.3.1 and it works great. Have updated my comment.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, I have the change locally. Will push after this CI run finishes just to make sure there are no issues.


.. code-block:: console

$ docker run --rm -it --network envoymesh wurstmeister/kafka:2.11-2.0.0 /opt/kafka/bin/kafka-console-consumer.sh --bootstrap-server envoy:19092 --topic test_topic --from-beginning
Copy link
Contributor

Choose a reason for hiding this comment

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

See comment above re: version

- "2181"

kafka:
image: wurstmeister/kafka:2.11-2.0.0
Copy link
Contributor

Choose a reason for hiding this comment

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

See comment above re: version

- "8001:8001"

zookeeper:
image: wurstmeister/zookeeper:3.4.6
Copy link
Contributor

@moderation moderation Jan 25, 2020

Choose a reason for hiding this comment

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

Update to zookeeper:3.5.6 which is is the official certified Zookeeper image and has a nice side effect of being more than half the size of the old wurstmeister image. https://hub.docker.com/_/zookeeper?tab=tags. I've confirmed this image works in the sandbox in testing.

Copy link
Author

Choose a reason for hiding this comment

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

I am reluctant to use latest tags in general. And the version difference is not very significant so I'd prefer to leave it as is unless you have a strong opinion about this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just updated my comment to suggest using the official certified Zookeeper image. Agree with avoiding :latest. I tested with this image and 👍

@mattklein123
Copy link
Member

@amre-abb sorry looks like DCO is broken. Can you do a force push to fix and we can get this merged? Thank you!

/wait

@mattklein123
Copy link
Member

DCO still needs fixing. :(

/wait

@stale
Copy link

stale bot commented Feb 3, 2020

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot added the stale stalebot believes this issue/PR has not been touched recently label Feb 3, 2020
@stale
Copy link

stale bot commented Feb 10, 2020

This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot closed this Feb 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale stalebot believes this issue/PR has not been touched recently waiting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants