apache
AVRO-3603 dotnet reflect reader and writer refactoring
Jira
- [ ] My PR addresses the following Avro Jira issues and references them in the PR title.
Tests
My PR do not add or update functionality, there is no new tests
Commits
- [ ] My commits all reference Jira issues in their subject lines.
Documentation
Readme file is updated https://github.com/KhrystynaPopadyuk/avro/tree/avro-3603-dotnet-reflect-refactoring/lang/csharp/src/apache/main/Reflect
Hi @KalleOlaviNiemitalo and @RyanSkraba ,
Please review my new PR. This is refactoring to reduce static class and fields and introduce DI. This PR will allow further refactoring in future.
Thanks.
Does this change help users of the library in some way? Better support for unloadable assemblies perhaps?
The breaking changes are worrisome, but I'm not using Avro.Reflect directly and https://github.com/confluentinc/confluent-kafka-dotnet/tree/v1.9.2/src/Confluent.SchemaRegistry.Serdes.Avro doesn't appear to use it either (uses only Avro.Generic and Avro.Specific).
Can the dependency on Microsoft.Extensions.DependencyInjection be replaced with Microsoft.Extensions.DependencyInjection.Abstractions? For people using Autofac or other DI libraries.
Can Avro.Reflect be split to a separate NuGet package so that users of Avro.Generic and Avro.Specific don't need to ship any DI libraries (and audit compliance with their licenses)?
For Microsoft.Extensions.DependencyInjection.Abstractions, I'd prefer the latest version in the 2.1.* series rather than 6.0.0. It is supported on .NET Framework as described in https://dotnet.microsoft.com/en-us/platform/support/policy/aspnetcore-2.1, and I suspect this support will continue after .NET 6.0 support ends. Developers of applications that target .NET 6.0 can add a dependency on the 6.0.0 package if they want.
Re the licensing, my primary concern is that it's difficult to be certain that I'm shipping all the required third-party notices with my applications. I assume Apache would likewise have to find the third-party notices applicable to the DI library and add them to the Apache.Avro.Tools package, because that package would contain the library.
… unlike the Apache.Avro package, which would just reference the DI library rather than include it.
Can Avro.Reflect be split to a separate NuGet package so that users of Avro.Generic and Avro.Specific don't need to ship any DI libraries (and audit compliance with their licenses)?
Looking at this PR a bit more, Avro.Reflect.DependencyInjection.IServiceCollectionExtensions is the only class that references Microsoft.Extensions.DependencyInjection or IServiceProvider, and its implementation is rather trivial. The dependency on the DI library could be avoided by deleting IServiceCollectionExtensions and making class ReflectCache public like class ClassCache was.
Hi @KalleOlaviNiemitalo and @martin-g ,
Thank you for review and feedback.
I believe that at the end of day such refactoring is important and would be beneficial for both contributors (provide easier maintenance and extensibility) and end users (make it flexible, allow inject custom logic and override default behavior). There is no goal to change serialization and deserialization. All changes are around reflection.
From your comments I understand that there are two main concerns: breaking changes and new dependency.
I agree with @KalleOlaviNiemitalo that we can easy resolve new dependency issue - do not add DI, create new project or leave it with end user.
About breaking changes. After second thought it looks like there is way to do refactoring without introducing breaking changes. The example and final state can be reviewed at https://github.com/apache/avro/pull/1823 (this PR is for reference only). As you can see in case if there is no breaking changes there are much more updates, that PR is massive.
I do not think that it is safe to deliver such massive PR. Also it is difficult to review them. Likely we can break down it to small pieces, implement, review and deliver it one by one. First such update are created and ready for review: https://github.com/apache/avro/pull/1824.
Thank you for your time. Khrystyna