[GitHub] [ignite] ptupitsyn commented on a change in pull request #9628: IGNITE-14299 .NET: Support BinaryArrays in services

[GitBox <gi...@apache.org>]

ptupitsyn commented on a change in pull request #9628:
URL: https://github.com/apache/ignite/pull/9628#discussion_r765091305



##########
File path: modules/platforms/dotnet/Apache.Ignite.Core.Tests/Client/Services/ServicesClientTest.cs
##########
@@ -585,4 +594,26 @@ private static IServices ServerServices
             get { return Ignition.GetIgnite().GetServices(); }
         }
     }
+
+    /// <summary>
+    /// Tests for <see cref="IServicesClient"/>.
+    /// </summary>
+    public class ServicesClientTestBinaryArrays : ServicesClientTest
+    {
+        /// <summary>Setup UseTypedArray flag value.</summary>
+        [TestFixtureSetUp]
+        public override void FixtureSetUp()
+        {
+            TestUtils.UseBinaryArray = true;

Review comment:
       Instead of using global mutable state in an unrelated class, I suggest to pass `useBinaryArray` as a constructor parameter like this:
   
   ```
   public class ServicesClientTestBinaryArrays : ServicesClientTest
   {
       public ServicesClientTestBinaryArrays() : base(useBinaryArray: true) {}
   }
   ```

##########
File path: modules/platforms/dotnet/Apache.Ignite.Core/Impl/Services/ServiceProxyInvoker.cs
##########
@@ -102,6 +102,12 @@ internal static class ServiceProxyInvoker
                         methodName, svcType, argsLength));
 
             // 2) There is more than 1 method with specified name - resolve with argument types.
+            methods = methods.Where(m => AreMethodArgsCompatible(arguments, m.GetParameters(), true)).ToArray();

Review comment:
       I tried reverting all changes in this file, and all tests pass. Please double check - either we need more tests, or those changes are unnecessary.




-- 
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: notifications-unsubscribe@ignite.apache.org

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