Skip to content

Adding the support for key for trigger & output binding #328

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

Merged
merged 20 commits into from
Aug 3, 2022

Conversation

shrohilla
Copy link
Contributor

@shrohilla shrohilla commented Apr 29, 2022

  • Adding the support for key for trigger & output binding as per customer demand :-
  1. Key is always None in Python binding #326
  2. The KafkaTrigger input parameter doesn't expose the (consumeResult.Message.)Key property  azure-functions-dotnet-worker#848

Kafka supported any library provide customer the support of reading key & value(message), so this is a required support for the customer

  • Unit testing -- done
  • E2E testing -- the language worker is still in-progress, will add the test case when that will be done

@shrohilla shrohilla marked this pull request as draft April 29, 2022 16:15
@shrohilla
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@shrohilla shrohilla requested a review from TsuyoshiUshio May 2, 2022 18:23
@shrohilla shrohilla changed the title adding the code required Adding the support for key for trigger & output binding May 2, 2022
@shrohilla shrohilla marked this pull request as ready for review May 2, 2022 18:23
@shrohilla shrohilla marked this pull request as draft May 2, 2022 18:30
@shrohilla shrohilla marked this pull request as ready for review May 4, 2022 19:53
@shrohilla shrohilla requested a review from krishna-kariya May 10, 2022 08:30
Copy link
Contributor

@TsuyoshiUshio TsuyoshiUshio left a comment

Choose a reason for hiding this comment

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

You might add a simple unit testing for the new logic.

@shrohilla shrohilla requested a review from lpapudippu August 3, 2022 15:23
Copy link
Contributor

@krishna-kariya krishna-kariya left a comment

Choose a reason for hiding this comment

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

Should we set the Key as well in the constructor at line 110 in KafkaEventData.cs?

@shrohilla
Copy link
Contributor Author

Should we set the Key as well in the constructor at line 110 in KafkaEventData.cs?

Yes, even if the key is null it will write the key as null and it's customer choice in his code if it wants to read that or not.

Copy link
Contributor

@lpapudippu lpapudippu left a comment

Choose a reason for hiding this comment

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

LGTM

@shrohilla shrohilla merged commit 356ca36 into dev Aug 3, 2022
@shrohilla shrohilla deleted the shrohilla/add-key-support branch August 3, 2022 17:13
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

Successfully merging this pull request may close these issues.

4 participants