You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@avro.apache.org by GitBox <gi...@apache.org> on 2022/08/11 03:42:19 UTC

[GitHub] [avro] KhrystynaPopadyuk opened a new pull request, #1819: AVRO-3603 dotnet reflect reader and writer refactoring

KhrystynaPopadyuk opened a new pull request, #1819:
URL: https://github.com/apache/avro/pull/1819

   Make sure you have checked _all_ steps below.
   
   ### Jira
   
   - [ ] My PR addresses the following [Avro Jira](https://issues.apache.org/jira/browse/AVRO/) issues and references them in the PR title. For example, "AVRO-1234: My Avro PR"
     - https://issues.apache.org/jira/browse/AVRO-XXX
     - In case you are adding a dependency, check if the license complies with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
   
   ### Tests
   
   - [ ] My PR adds the following unit tests __OR__ does not need testing for this extremely good reason:
   
   ### Commits
   
   - [ ] My commits all reference Jira issues in their subject lines. In addition, my commits follow the guidelines from "[How to write a good git commit message](https://chris.beams.io/posts/git-commit/)":
     1. Subject is separated from body by a blank line
     1. Subject is limited to 50 characters (not including Jira issue reference)
     1. Subject does not end with a period
     1. Subject uses the imperative mood ("add", not "adding")
     1. Body wraps at 72 characters
     1. Body explains "what" and "why", not "how"
   
   ### Documentation
   
   - [ ] In case of new functionality, my PR adds documentation that describes how to use it.
     - All the public functions and the classes in the PR contain Javadoc that explain what it does
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@avro.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] martin-g commented on a diff in pull request #1819: AVRO-3603 dotnet reflect reader and writer refactoring

Posted by GitBox <gi...@apache.org>.
martin-g commented on code in PR #1819:
URL: https://github.com/apache/avro/pull/1819#discussion_r943168006


##########
lang/csharp/src/apache/main/Reflect/EnumCache.cs:
##########
@@ -1,57 +0,0 @@
-/*  
- * Licensed to the Apache Software Foundation (ASF) under one
- * or more contributor license agreements.  See the NOTICE file
- * distributed with this work for additional information
- * regarding copyright ownership.  The ASF licenses this file
- * to you under the Apache License, Version 2.0 (the
- * "License"); you may not use this file except in compliance
- * with the License.  You may obtain a copy of the License at
- *
- *     https://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-using System;
-using System.Collections.Concurrent;
-using Avro;
-
-namespace Avro.Reflect
-{
-    /// <summary>
-    /// Cache of enum types. Cache key is the schema fullname.
-    /// </summary>
-    public static class EnumCache

Review Comment:
   Is it possible to keep the class for a while as deprecated ?



##########
lang/csharp/src/apache/main/Reflect/Converter/DateTimeOffsetToLongConverter.cs:
##########
@@ -17,8 +17,9 @@
  */
 
 using System;
+using Avro.Reflect.Interface;
 
-namespace Avro.Reflect
+namespace Avro.Reflect.Conver

Review Comment:
   missing `er` ?!



##########
lang/csharp/src/apache/main/Reflect/Reflection/DotnetClass.cs:
##########
@@ -20,8 +20,9 @@
 using System.Reflection;
 using System.Collections.Concurrent;
 using Avro;
+using Avro.Reflect.Interface;
 
-namespace Avro.Reflect
+namespace Avro.Reflect.Reflection

Review Comment:
   Why there are two `reflect***` packages/namespaces here ?
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] KhrystynaPopadyuk commented on a diff in pull request #1819: AVRO-3603 dotnet reflect reader and writer refactoring

Posted by GitBox <gi...@apache.org>.
KhrystynaPopadyuk commented on code in PR #1819:
URL: https://github.com/apache/avro/pull/1819#discussion_r943126289


##########
lang/csharp/src/apache/main/Reflect/ReflectDefaultWriter.cs:
##########
@@ -188,9 +181,9 @@
                     return obj is string;
                 case Schema.Type.Error:
                 case Schema.Type.Record:
-                    return _classCache.GetClass(sc as RecordSchema).GetClassType() == obj.GetType();
+                    return _reflectCache.GetClass(sc as RecordSchema).GetClassType() == obj.GetType();

Review Comment:
   This is existing logic.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] KhrystynaPopadyuk commented on pull request #1819: AVRO-3603 dotnet reflect reader and writer refactoring

Posted by GitBox <gi...@apache.org>.
KhrystynaPopadyuk commented on PR #1819:
URL: https://github.com/apache/avro/pull/1819#issuecomment-1211613451

   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.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] KhrystynaPopadyuk commented on a diff in pull request #1819: AVRO-3603 dotnet reflect reader and writer refactoring

Posted by GitBox <gi...@apache.org>.
KhrystynaPopadyuk commented on code in PR #1819:
URL: https://github.com/apache/avro/pull/1819#discussion_r943126767


##########
lang/csharp/src/apache/main/Reflect/Reflection/ReflectCache.cs:
##########
@@ -19,63 +19,31 @@
 using System;
 using System.Collections;
 using System.Collections.Concurrent;
+using System.Collections.Generic;
+using System.Text;
+using Avro.Reflect.Converter;
+using Avro.Reflect.Interface;
 
-namespace Avro.Reflect
+namespace Avro.Reflect.Reflection
 {
-    /// <summary>
-    /// Class holds a cache of C# classes and their properties. The key for the cache is the schema full name.
-    /// </summary>
-    public class ClassCache
+    internal class ReflectCache : IReflectCache
     {
-        private static ConcurrentBag<IAvroFieldConverter> _defaultConverters = new ConcurrentBag<IAvroFieldConverter>();
-
+        private ConcurrentDictionary<string, Type> _nameEnumMap = new ConcurrentDictionary<string, Type>();

Review Comment:
   Done



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] github-code-scanning[bot] commented on a diff in pull request #1819: AVRO-3603 dotnet reflect reader and writer refactoring

Posted by GitBox <gi...@apache.org>.
github-code-scanning[bot] commented on code in PR #1819:
URL: https://github.com/apache/avro/pull/1819#discussion_r943108992


##########
lang/csharp/src/apache/main/Reflect/ReflectDefaultWriter.cs:
##########
@@ -188,9 +181,9 @@
                     return obj is string;
                 case Schema.Type.Error:
                 case Schema.Type.Record:
-                    return _classCache.GetClass(sc as RecordSchema).GetClassType() == obj.GetType();
+                    return _reflectCache.GetClass(sc as RecordSchema).GetClassType() == obj.GetType();
                 case Schema.Type.Enumeration:
-                    return EnumCache.GetEnumeration(sc as EnumSchema) == obj.GetType();
+                    return _reflectCache.GetEnumeration(sc as EnumSchema) == obj.GetType();

Review Comment:
   ## Dereferenced variable may be null
   
   Variable [obj](1) may be null here as suggested by [this](2) null check.
   Variable [obj](1) may be null here as suggested by [this](3) null check.
   
   [Show more details](https://github.com/apache/avro/security/code-scanning/2845)



##########
lang/csharp/src/apache/main/Reflect/Reflection/ReflectCache.cs:
##########
@@ -288,17 +256,53 @@
~                    else
~                    {
~                        // check the schema types are registered
~                        foreach (var o in us.Schemas)
~                        {
~                            if (o.Tag == Schema.Type.Record && GetClass(o as RecordSchema) == null)
~                            {
~                                throw new AvroException($"Class for union record type {o.Fullname} is not registered. Create a ClassCache object and call LoadClassCache");
~                            }
~                        }
~                    }
~
                     break;
             }
         }
+
+        /// <summary>
+        /// Lookup an entry in the cache - based on the schema fullname
+        /// </summary>
+        /// <param name="schema"></param>
+        /// <returns></returns>
+        public Type GetEnumeration(NamedSchema schema)
+        {
+            Type t;
+            if (!_nameEnumMap.TryGetValue(schema.Fullname, out t))
+            {
+                throw new AvroException($"Couldn't find enumeration for avro fullname: {schema.Fullname}");
+            }
+
+            return t;
+        }
+
+         private void AddEnumNameMapItem(NamedSchema schema, Type dotnetEnum)
+        {
+            _nameEnumMap.TryAdd(schema.Fullname, dotnetEnum);
+        }
+
+        private void AddClassNameMapItem(RecordSchema schema, Type dotnetClass)
+        {
+            if (schema != null && GetClass(schema) != null)
+            {
+                return;
+            }
+
+            if (!dotnetClass.IsClass)
+            {
+                throw new AvroException($"Type {dotnetClass.Name} is not a class");
+            }
+
+            _nameClassMap.TryAdd(schema.Fullname, new DotnetClass(dotnetClass, schema, this));

Review Comment:
   ## Dereferenced variable may be null
   
   Variable [schema](1) may be null here as suggested by [this](2) null check.
   
   [Show more details](https://github.com/apache/avro/security/code-scanning/2847)



##########
lang/csharp/src/apache/main/Reflect/Reflection/ReflectCache.cs:
##########
@@ -19,63 +19,31 @@
 using System;
 using System.Collections;
 using System.Collections.Concurrent;
+using System.Collections.Generic;
+using System.Text;
+using Avro.Reflect.Converter;
+using Avro.Reflect.Interface;
 
-namespace Avro.Reflect
+namespace Avro.Reflect.Reflection
 {
-    /// <summary>
-    /// Class holds a cache of C# classes and their properties. The key for the cache is the schema full name.
-    /// </summary>
-    public class ClassCache
+    internal class ReflectCache : IReflectCache
     {
-        private static ConcurrentBag<IAvroFieldConverter> _defaultConverters = new ConcurrentBag<IAvroFieldConverter>();
-
+        private ConcurrentDictionary<string, Type> _nameEnumMap = new ConcurrentDictionary<string, Type>();

Review Comment:
   ## Missed 'readonly' opportunity
   
   Field '_nameEnumMap' can be 'readonly'.
   
   [Show more details](https://github.com/apache/avro/security/code-scanning/2850)



##########
lang/csharp/src/apache/main/Reflect/ReflectDefaultWriter.cs:
##########
@@ -188,9 +181,9 @@
                     return obj is string;
                 case Schema.Type.Error:
                 case Schema.Type.Record:
-                    return _classCache.GetClass(sc as RecordSchema).GetClassType() == obj.GetType();
+                    return _reflectCache.GetClass(sc as RecordSchema).GetClassType() == obj.GetType();

Review Comment:
   ## Dereferenced variable may be null
   
   Variable [obj](1) may be null here as suggested by [this](2) null check.
   Variable [obj](1) may be null here as suggested by [this](3) null check.
   
   [Show more details](https://github.com/apache/avro/security/code-scanning/2844)



##########
lang/csharp/src/apache/main/Reflect/ReflectDefaultWriter.cs:
##########
@@ -28,28 +30,19 @@
     /// </summary>
     public class ReflectDefaultWriter : SpecificDefaultWriter
     {
-        private ClassCache _classCache = new ClassCache();
-
-        /// <summary>
-        /// Class cache
-        /// </summary>
-        public ClassCache ClassCache { get => _classCache; }
+        private IReflectCache _reflectCache;

Review Comment:
   ## Missed 'readonly' opportunity
   
   Field '_reflectCache' can be 'readonly'.
   
   [Show more details](https://github.com/apache/avro/security/code-scanning/2849)



##########
lang/csharp/src/apache/main/Reflect/ReflectDefaultReader.cs:
##########
@@ -37,12 +39,7 @@
         /// </summary>
         public Type MapType { get => _mapType; set => _mapType = value; }
 
-        private ClassCache _classCache = new ClassCache();
-
-        /// <summary>
-        /// Class cache
-        /// </summary>
-        public ClassCache ClassCache { get => _classCache; }
+        private IReflectCache _reflectCache;

Review Comment:
   ## Missed 'readonly' opportunity
   
   Field '_reflectCache' can be 'readonly'.
   
   [Show more details](https://github.com/apache/avro/security/code-scanning/2848)



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] KalleOlaviNiemitalo commented on pull request #1819: AVRO-3603 dotnet reflect reader and writer refactoring

Posted by GitBox <gi...@apache.org>.
KalleOlaviNiemitalo commented on PR #1819:
URL: https://github.com/apache/avro/pull/1819#issuecomment-1211616306

   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)?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] KhrystynaPopadyuk commented on a diff in pull request #1819: AVRO-3603 dotnet reflect reader and writer refactoring

Posted by GitBox <gi...@apache.org>.
KhrystynaPopadyuk commented on code in PR #1819:
URL: https://github.com/apache/avro/pull/1819#discussion_r943126632


##########
lang/csharp/src/apache/main/Reflect/ReflectDefaultReader.cs:
##########
@@ -37,12 +39,7 @@
         /// </summary>
         public Type MapType { get => _mapType; set => _mapType = value; }
 
-        private ClassCache _classCache = new ClassCache();
-
-        /// <summary>
-        /// Class cache
-        /// </summary>
-        public ClassCache ClassCache { get => _classCache; }
+        private IReflectCache _reflectCache;

Review Comment:
   Done



##########
lang/csharp/src/apache/main/Reflect/ReflectDefaultWriter.cs:
##########
@@ -28,28 +30,19 @@
     /// </summary>
     public class ReflectDefaultWriter : SpecificDefaultWriter
     {
-        private ClassCache _classCache = new ClassCache();
-
-        /// <summary>
-        /// Class cache
-        /// </summary>
-        public ClassCache ClassCache { get => _classCache; }
+        private IReflectCache _reflectCache;

Review Comment:
   Done



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] KalleOlaviNiemitalo commented on pull request #1819: AVRO-3603 dotnet reflect reader and writer refactoring

Posted by GitBox <gi...@apache.org>.
KalleOlaviNiemitalo commented on PR #1819:
URL: https://github.com/apache/avro/pull/1819#issuecomment-1211831944

   … unlike the Apache.Avro package, which would just reference the DI library rather than include it.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] KhrystynaPopadyuk commented on a diff in pull request #1819: AVRO-3603 dotnet reflect reader and writer refactoring

Posted by GitBox <gi...@apache.org>.
KhrystynaPopadyuk commented on code in PR #1819:
URL: https://github.com/apache/avro/pull/1819#discussion_r943126289


##########
lang/csharp/src/apache/main/Reflect/ReflectDefaultWriter.cs:
##########
@@ -188,9 +181,9 @@
                     return obj is string;
                 case Schema.Type.Error:
                 case Schema.Type.Record:
-                    return _classCache.GetClass(sc as RecordSchema).GetClassType() == obj.GetType();
+                    return _reflectCache.GetClass(sc as RecordSchema).GetClassType() == obj.GetType();

Review Comment:
   This is existing logic. Should not be updated in scope of AVRO-3603



##########
lang/csharp/src/apache/main/Reflect/ReflectDefaultWriter.cs:
##########
@@ -188,9 +181,9 @@
                     return obj is string;
                 case Schema.Type.Error:
                 case Schema.Type.Record:
-                    return _classCache.GetClass(sc as RecordSchema).GetClassType() == obj.GetType();
+                    return _reflectCache.GetClass(sc as RecordSchema).GetClassType() == obj.GetType();
                 case Schema.Type.Enumeration:
-                    return EnumCache.GetEnumeration(sc as EnumSchema) == obj.GetType();
+                    return _reflectCache.GetEnumeration(sc as EnumSchema) == obj.GetType();

Review Comment:
   This is existing logic. Should not be updated in scope of AVRO-3603



##########
lang/csharp/src/apache/main/Reflect/Reflection/ReflectCache.cs:
##########
@@ -288,17 +256,53 @@
~                    else
~                    {
~                        // check the schema types are registered
~                        foreach (var o in us.Schemas)
~                        {
~                            if (o.Tag == Schema.Type.Record && GetClass(o as RecordSchema) == null)
~                            {
~                                throw new AvroException($"Class for union record type {o.Fullname} is not registered. Create a ClassCache object and call LoadClassCache");
~                            }
~                        }
~                    }
~
                     break;
             }
         }
+
+        /// <summary>
+        /// Lookup an entry in the cache - based on the schema fullname
+        /// </summary>
+        /// <param name="schema"></param>
+        /// <returns></returns>
+        public Type GetEnumeration(NamedSchema schema)
+        {
+            Type t;
+            if (!_nameEnumMap.TryGetValue(schema.Fullname, out t))
+            {
+                throw new AvroException($"Couldn't find enumeration for avro fullname: {schema.Fullname}");
+            }
+
+            return t;
+        }
+
+         private void AddEnumNameMapItem(NamedSchema schema, Type dotnetEnum)
+        {
+            _nameEnumMap.TryAdd(schema.Fullname, dotnetEnum);
+        }
+
+        private void AddClassNameMapItem(RecordSchema schema, Type dotnetClass)
+        {
+            if (schema != null && GetClass(schema) != null)
+            {
+                return;
+            }
+
+            if (!dotnetClass.IsClass)
+            {
+                throw new AvroException($"Type {dotnetClass.Name} is not a class");
+            }
+
+            _nameClassMap.TryAdd(schema.Fullname, new DotnetClass(dotnetClass, schema, this));

Review Comment:
   This is existing logic. Should not be updated in scope of AVRO-3603



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] KalleOlaviNiemitalo commented on pull request #1819: AVRO-3603 dotnet reflect reader and writer refactoring

Posted by GitBox <gi...@apache.org>.
KalleOlaviNiemitalo commented on PR #1819:
URL: https://github.com/apache/avro/pull/1819#issuecomment-1211651561

   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.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] KhrystynaPopadyuk closed pull request #1819: AVRO-3603 dotnet reflect reader and writer refactoring

Posted by GitBox <gi...@apache.org>.
KhrystynaPopadyuk closed pull request #1819: AVRO-3603 dotnet reflect reader and writer refactoring
URL: https://github.com/apache/avro/pull/1819


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@avro.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] KalleOlaviNiemitalo commented on pull request #1819: AVRO-3603 dotnet reflect reader and writer refactoring

Posted by GitBox <gi...@apache.org>.
KalleOlaviNiemitalo commented on PR #1819:
URL: https://github.com/apache/avro/pull/1819#issuecomment-1211624102

   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.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] KalleOlaviNiemitalo commented on pull request #1819: AVRO-3603 dotnet reflect reader and writer refactoring

Posted by GitBox <gi...@apache.org>.
KalleOlaviNiemitalo commented on PR #1819:
URL: https://github.com/apache/avro/pull/1819#issuecomment-1211856169

   > 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.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] KhrystynaPopadyuk commented on pull request #1819: AVRO-3603 dotnet reflect reader and writer refactoring

Posted by GitBox <gi...@apache.org>.
KhrystynaPopadyuk commented on PR #1819:
URL: https://github.com/apache/avro/pull/1819#issuecomment-1212797034

   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. 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. 
   
   Thanks one more time for your time.
   Khrystyna
   
   
   
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org