BUG: Remove nullability from attribute properties

[gliljas]

As mentioned in #175, attributes can't have nullable properties. It doesn't compile. It seems you haven't tested it.
That PR used nullable backing fields and GetOrDefault to retrieve them. I think that may actually be better for the future, enabling properties to be set in different levels, but here I took the easy route.

Also note that the default RequestTimeoutMs in this library is 5000ms, whereas in the Confluent library, if not set, it's 30000ms. Since it was impossible to specify RequestTimeoutMs before this fix, this would actually mean that we're changing from 30000 to 5000 now. The documentation for Confluent's MessageSendMaxRetries claims that the default is int.MaxValue, but I doubt that's the truth. confluentinc/confluent-kafka-dotnet#1858

Edit: It was the truth

[gliljas]

int.MaxValue

[gliljas]

@shrohilla I would really appreciate a bit of feedback on a bunch of PR:s. This one is a very clear bug that was reported more than 2 years ago.

[shrohilla]

@gliljas could you please explain more details about this PR, is default value is not being reflected for all attributes or is this only for int max value is assigned to MaxRetries attribute on startup ??

[gliljas]

The PR is about having nullable properties on attributes, i.e MaxMessageBytes, BatchSize, EnableIdempotence, MessageTimeoutMs, RequestTimeoutMs or MaxRetries. It's not allowed. Your code will not compile if you use any of these properties.

[shrohilla]

@gliljas could you please share the sample code where you found there was code compilation issue with functions ??
As per my understanding, the default value should be assigned to these properties if they are not set

[gliljas]

Well, basically you could just try to use any of the mentioned properties yourself, and you'll see that it doesn't work.
Attribute property values must be compilable to constants. Nullables aren't.

https://dotnetfiddle.net/w5NrwO

[shrohilla]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[shrohilla]

@gliljas Thanks alot for your contribution.
There is also regression with the change, I would request you to please fix that.

I'll also request you to please add 1 or 2 E2E tests with the change for both E2E and Lang E2E for testing the regression.

[gliljas]

I doubt assigning the default values only will work.

That is not what the code does. The property types have changed. The end result is that the Confluent library will always be populated with the default values specified here, instead of Confluent's default values, so they have to be specified. In the case of MaxRetries, there is a difference, since Confluent specified int.MaxValue but this library specifies 2.

Another option, which I used in a PR two years ago is to use nullable backingfields, which will only get a value if they're specifically set.

[gliljas]

I'll check the regression first.

[shrohilla]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[shrohilla]

@gliljas I would request you to add 1 End to End test case to validate the flow

[shrohilla]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[gliljas]

Sure thing, but any suggestion what such a test would do and assert? :)

[shrohilla]

Sure thing, but any suggestion what such a test would do and assert? :)

One way could be try to re ingest the same event using the output binding after enabling idempotency than producer will not allow to re-ingest the same event and throw the error, that error you need to read from the log -- https://github.com/Azure/azure-functions-kafka-extension/blob/dev/test/Microsoft.Azure.WebJobs.Extensions.Kafka.EndToEndTests/TestLoggerProvider.cs#L42

Please share the feedback if this proposal doesn't works out.

[shrohilla]

@gliljas any update on this ??

[gliljas]

I'll get back to it tonight.

[shrohilla]

@gliljas any update on this ??

[gliljas]

Just making sure it was noted that an e2e test was added. Testing idempotency is a bit too hidden in Kafka to be triggered from the outside, so I chose MaxMessageBytes instead.

[shrohilla]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[shrohilla]

LGTM

[shrohilla]

@gliljas Thanks alot for your contribution we will plan this change in next release.