Kafka Extension upgrade from 3.6.0 to 3.7.0 & Fixing the null attribute issue #1232

shrohilla · 2022-12-14T13:12:58Z

Issue describing the changes in this PR

resolves #issue_for_this_pr

Pull request checklist

My changes do not require documentation changes
- Otherwise: Documentation issue linked to PR
My changes should not be added to the release notes for the next release
- Otherwise: I've added my notes to release_notes.md
My changes do not need to be backported to a previous version
- Otherwise: Backport tracked by issue/PR #issue_or_pr
I have added all required tests (Unit tests, E2E tests)

Additional information

Additional PR information

raorugan · 2023-01-02T07:36:37Z

@kshyju - can you please approve this PR as it is pending for out-proc worker extension nuget package release?

kshyju

Noticed some public methods/types are missing comments/summary. Could we add that as well? ex: https://github.com/Azure/azure-functions-dotnet-worker/blob/main/extensions/Worker.Extensions.Kafka/src/KafkaTriggerAttribute.cs#L15

kshyju · 2023-01-03T17:26:39Z

extensions/Worker.Extensions.Kafka/src/KafkaOutputAttribute.cs

@@ -32,38 +32,38 @@ public KafkaOutputAttribute(string brokerList, string topic)
        /// Gets or sets the Avro schema.
        /// Should be used only if a generic record should be generated
        /// </summary>
-        public string? AvroSchema { get; set; }
+        public string AvroSchema { get; set; }


Why changing from nullable to non nullable? Does tooling/static analysis shows a warning about this (non nullable)prop not being initialized? I think it is best to embrace nullable type feature the language is offering.

Will customers start seeing warnings about props not being initialized after they upgrade from 3.6.0 to 3.70?

The reason for the change is the compilation error with use of nullable attributes as discussed here.
Agreed that we should embrace the nullable types - something we will definitely look into going forward. But for now - In order to maintain parity with the kafka extension we are keeping the attributes non-nullable.
Regarding warnings to customers - No customers will not see any warnings about the property not being initialized this being an optional parameter for the attribute supported by C#.

kshyju · 2023-01-03T17:28:23Z

extensions/Worker.Extensions.Kafka/src/KafkaOutputAttribute.cs

@@ -32,38 +32,38 @@ public KafkaOutputAttribute(string brokerList, string topic)
        /// Gets or sets the Avro schema.
        /// Should be used only if a generic record should be generated
        /// </summary>
-        public string? AvroSchema { get; set; }
+        public string AvroSchema { get; set; }

        /// <summary>
        /// Gets or sets the Maximum transmit message size. Default: 1MB


nit:
Use lowercase maximum.
Consider using consistent casing across the file (ex: default or Default?).
Consider rephrasing to include the unit as well (like "in bytes").

kshyju · 2023-01-03T17:34:45Z

extensions/Worker.Extensions.Kafka/src/KafkaOutputAttribute.cs


        /// <summary>
        /// How many times to retry sending a failing Message. **Note:** default: 2 
        /// </summary>
        /// <remarks>Retrying may cause reordering unless <c>EnableIdempotence</c> is set to <c>true</c>.</remarks>
-        public int? MaxRetries { get; set; }
+        public int MaxRetries { get; set; } = int.MaxValue;


2 billion max retries? 😵

This is in line with the extension and the underlying Confluent Kafka sdk. Even when set to null this is the value taken in the underlying sdk. Explicitly setting as default will help customers.

kshyju · 2023-01-03T17:39:21Z

extensions/Worker.Extensions.Kafka/src/KafkaTriggerAttribute.cs


        /// <summary>
        /// Path to client's certificate.
        /// ssl.certificate.location in librdkafka
        /// </summary>
-        public string? SslCertificateLocation { get; set; }
+        public string SslCertificateLocation { get; set; }

        /// <summary>
        /// Password for client's certificate.
        /// ssl.key.password in librdkafka


Q: Is this a valid comment to present here?

Comment in line with the extension. https://github.com/Azure/azure-functions-kafka-extension/blob/82de33298da77b7e5066db8d6b902370641ae765/src/Microsoft.Azure.WebJobs.Extensions.Kafka/Output/KafkaAttribute.cs#L132

kshyju · 2023-01-03T17:54:39Z

extensions/Worker.Extensions.Kafka/src/KafkaOutputAttribute.cs


        /// <summary>
        /// Local message timeout. This value is only enforced locally and limits the time a produced message waits for successful delivery. A time of 0 is infinite. This is the maximum time used to deliver a message (including retries). Delivery error occurs when either the retry count or the message timeout are exceeded. default: 300000
        /// </summary>
-        public int? MessageTimeoutMs { get; set; }
+        public int MessageTimeoutMs { get; set; } = 300_000;


Question: Would this change be considered as a breaking change? Is the code which consumes this property (may be in host/kafka webjobs extn) updated to handle the default value change? Earlier this was sent as null and now it will send 300000 when sending the serialized version of this class instance.

No. This would not be a breaking change. When set to null the value used was 300000 in the underlying librdkafka library.

kshyju · 2023-01-18T22:35:02Z

@raorugan @shrohilla Ping on this PR.

fabiocav · 2023-01-19T01:29:38Z

@shrohilla / @raorugan can you take a look at the comments above? Thanks!

ghost · 2023-01-26T04:01:38Z

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 7 days of this comment.

jainharsh98 · 2023-02-06T06:25:01Z

@kshyju The Kafka Extension pipeline seems to be waiting for some permission. Can you please help with that?

kshyju · 2023-02-06T19:10:08Z

@kshyju The Kafka Extension pipeline seems to be waiting for some permission. Can you please help with that?

All good now. https://azfunc.visualstudio.com/Azure%20Functions/_build/results?buildId=113680&view=results

jainharsh98 · 2023-02-13T17:07:31Z

Closing in favor of #1336

shrohilla added 4 commits November 4, 2022 18:33

upgrade to 3.6.0

4d0509e

incorporating the review comment

e52f07f

upgrade kafka extension from 3.6.0 to 3.7.0

57b6a3d

added the required release notes

3a9e794

shrohilla marked this pull request as draft December 14, 2022 13:13

fixing the conflict

8d71de3

shrohilla marked this pull request as ready for review December 25, 2022 13:14

shrohilla requested a review from kshyju December 25, 2022 13:14

kshyju reviewed Jan 3, 2023

View reviewed changes

fabiocav added the Needs: Author Feedback label Jan 19, 2023

ghost added the no-recent-activity label Jan 26, 2023

ghost closed this Feb 2, 2023

jainharsh98 reopened this Feb 6, 2023

ghost removed the no-recent-activity label Feb 6, 2023

Improving summaries for attributes/methods

04fc775

kshyju requested a review from satvu February 6, 2023 17:07

satvu approved these changes Feb 6, 2023

View reviewed changes

jainharsh98 mentioned this pull request Feb 13, 2023

Kafka Extension upgrade from 3.6.0 to 3.7.0 & Fixing the null attribute issue #1336

Merged

7 tasks

jainharsh98 closed this Feb 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kafka Extension upgrade from 3.6.0 to 3.7.0 & Fixing the null attribute issue #1232

Kafka Extension upgrade from 3.6.0 to 3.7.0 & Fixing the null attribute issue #1232

shrohilla commented Dec 14, 2022 •

edited

Loading

raorugan commented Jan 2, 2023

kshyju left a comment

kshyju Jan 3, 2023

jainharsh98 Feb 6, 2023 •

edited

Loading

kshyju Jan 3, 2023

jainharsh98 Feb 6, 2023

kshyju Jan 3, 2023

jainharsh98 Feb 6, 2023 •

edited

Loading

kshyju Jan 3, 2023

jainharsh98 Feb 6, 2023

kshyju Jan 3, 2023

jainharsh98 Feb 6, 2023

kshyju commented Jan 18, 2023

fabiocav commented Jan 19, 2023

ghost commented Jan 26, 2023

jainharsh98 commented Feb 6, 2023

kshyju commented Feb 6, 2023

jainharsh98 commented Feb 13, 2023

Kafka Extension upgrade from 3.6.0 to 3.7.0 & Fixing the null attribute issue #1232

Kafka Extension upgrade from 3.6.0 to 3.7.0 & Fixing the null attribute issue #1232

Conversation

shrohilla commented Dec 14, 2022 • edited Loading

Issue describing the changes in this PR

Pull request checklist

Additional information

raorugan commented Jan 2, 2023

kshyju left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jainharsh98 Feb 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jainharsh98 Feb 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kshyju commented Jan 18, 2023

fabiocav commented Jan 19, 2023

ghost commented Jan 26, 2023

jainharsh98 commented Feb 6, 2023

kshyju commented Feb 6, 2023

jainharsh98 commented Feb 13, 2023

shrohilla commented Dec 14, 2022 •

edited

Loading

jainharsh98 Feb 6, 2023 •

edited

Loading

jainharsh98 Feb 6, 2023 •

edited

Loading