-
Notifications
You must be signed in to change notification settings - Fork 75
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
Update Samples and documentation #140
Conversation
…n into tsushi/pythoncardinality
…n into tsushi/pythoncardinality
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nits to make the documentation better, otherwise LGTM :)
binding-library/python/azure/functions_extensions/kafka/kafka.py
Outdated
Show resolved
Hide resolved
* @param kafkaEventData | ||
* @param context | ||
*/ | ||
// @FunctionName("KafkaTrigger-Java") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mind remove the comments? Are e waiting for the java library to be deployed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have two functions KafkaTrigger-Java
and KafkaTrigger-Java-Many
The first one is a sample for accessing local clusters with Cardinality.ONE. The second one is a sample for accessing Confluent Cloud with Cardinality.MANY. For the other samples, I comment out the second one, since a lot of getting started customers don't have a Confluence Cloud. So I keep a sample for local Kafka Cluster and comment out the sample for Confluence Cloud. This sample is for Docker, It aims to deploy to the Function App. That means it can't access the local cluster. So I comment out the local cluster sample and enable the Confluent Cloud sample. Should I remove the local cluster sample instead?
context.getLogger().info(message); | ||
} | ||
} | ||
// @FunctionName("KafkaTrigger-Java-Many") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did we comment it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have three sample functions. KafkaOutput
, Kafka-Trigger-Java
, and Kafka-Trigger-Java-Many
. The Kafka-Trigger-Java-Many
is the sample for Cardinality.MANY and Confluent Cloud. So not many client don't have the Confluent Cloud account. Instead, for the others, it accesses to the local cluster that the DevContainer automatically starts.
That is why I comment on the Kafka-Trigger-Java-Many
not to throw an error if the customer doesn't configure Confluent Cloud settings, but I wanted to provide some complex use-case samples.
"topic": "%KafkaTopic%", | ||
"brokerList": "%KafkaBroker%", | ||
"consumerGroup": "%KafkaConsumerGroup%", | ||
"topic": "users", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we return the %% values?
"topic": "test_topic", | ||
"brokerList": "localhost:9092", | ||
"consumerGroup" : "default" | ||
"topic": "users", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add %% values?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have suggested some text changes to the Samples/Readme.cmd
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments, mostly nit-picking comments! Thanks!
Co-authored-by: Anirudh Garg <anirudhgarg@gmail.com>
…ctions-kafka-extension into tsushi/pythoncardinality
If this PR has been merged, I'll merge this PR. The PR for change on the azure-functions-python-library repo. |
I already fix all of the point of him
I update and refactor the whole samples.
What I did?
Review Direction
Sorry guys, this PR becomes big. However, the change for each language is not that big.
Could you help me to review it for language experts?
Python for @Hazhzeng
Could you have a look at the change of
Java/TypeScript for @mhoeger
Could you have a look a the change of
Java for @amamounelsayed
Overview/Structure for @anirudhgarg
You can find each samples.
TODO