Re: [PR] IGNITE-23703 .NET: Add IgniteClientGroup [ignite-3]

2024-11-22 Thread via GitHub


ptupitsyn merged PR #4763:
URL: https://github.com/apache/ignite-3/pull/4763


-- 
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-unsubscr...@ignite.apache.org

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



Re: [PR] IGNITE-23703 .NET: Add IgniteClientGroup [ignite-3]

2024-11-22 Thread via GitHub


gurustron commented on PR #4763:
URL: https://github.com/apache/ignite-3/pull/4763#issuecomment-2493116154

   > > Maybe follow the factory pattern
   > ...
   > Certainly less confusing than the factory that reuses objects.
   
   Had some wrong assumptions about inner workings of the client. Agree on the 
factory approach being confusing. 
   
   > Maybe we should use a different name, like `IgniteClientGroup`?
   
   Actually if we will return something which is not disposable then probably 
`Pool` is fine too. 
   
   Also maybe create an interface `IIgniteClientGroup`/`IgniteClientPool` and 
rename current implementation to `RoundRobinIgniteClientGroup` or something 
similar? So in future if there will be a more complex approach to rotation 
algorithm it would be easier to swap? Alternatively this can be solved by 
introducing `IgniteClientPoolConfiguration.Algorithm` setting but potentially 
it less fluent. 
   


-- 
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-unsubscr...@ignite.apache.org

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



Re: [PR] IGNITE-23703 .NET: Add IgniteClientGroup [ignite-3]

2024-11-22 Thread via GitHub


ptupitsyn commented on code in PR #4763:
URL: https://github.com/apache/ignite-3/pull/4763#discussion_r1853440050


##
modules/platforms/dotnet/Apache.Ignite/IgniteClientPool.cs:
##
@@ -0,0 +1,162 @@
+/*
+ * 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
+ *
+ *  http://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.
+ */
+
+namespace Apache.Ignite;
+
+using System;
+using System.Diagnostics.CodeAnalysis;
+using System.Linq;
+using System.Threading;
+using System.Threading.Tasks;
+using Internal;
+using Internal.Common;
+
+/// 
+/// Ignite client pool. Thread safe.
+/// 
+/// This pool creates up to  
Ignite clients and returns them in a round-robin fashion.
+/// Ignite clients are thread safe, so there is no rent/return semantics.
+/// 
+/// 
+/// Register as a singleton in DI container:
+/// 
+/// builder.Services.AddSingleton(_ => new IgniteClientPool(
+/// new IgniteClientPoolConfiguration
+/// {
+/// Size = 3,
+/// ClientConfiguration = new("localhost"),
+/// }));
+/// 
+/// Invoke from a controller:
+/// 
+/// public async Task Index([FromServices] 
IgniteClientPool ignite)
+/// {
+/// var client = await ignite.GetClientAsync();
+/// var tables = await client.Tables.GetTablesAsync();
+/// return Ok(tables);
+/// }
+/// 
+/// 
+/// 
+public sealed class IgniteClientPool : IDisposable
+{
+private readonly IgniteClientInternal?[] _clients;
+
+private readonly SemaphoreSlim _clientsLock = new(1);
+
+private int _disposed;
+
+private int _clientIndex;
+
+/// 
+/// Initializes a new instance of the  class.
+/// 
+/// Configuration.
+public IgniteClientPool(IgniteClientPoolConfiguration configuration)
+{
+IgniteArgumentCheck.NotNull(configuration);
+IgniteArgumentCheck.NotNull(configuration.ClientConfiguration);
+IgniteArgumentCheck.Ensure(configuration.Size > 0, 
nameof(configuration.Size), "Pool size must be positive.");
+
+Configuration = configuration;
+_clients = new IgniteClientInternal[configuration.Size];
+}
+
+/// 
+/// Gets the configuration.
+/// 
+public IgniteClientPoolConfiguration Configuration { get; }
+
+/// 
+/// Gets a value indicating whether the pool is disposed.
+/// 
+public bool IsDisposed => Interlocked.CompareExchange(ref _disposed, 0, 0) 
== 1;
+
+/// 
+/// Gets an Ignite client from the pool. Creates a new one if necessary.
+/// Performs round-robin balancing across pooled instances.
+/// 
+/// NOTE: Do not dispose the client instance returned by this method, it 
is managed by the pool.
+/// 
+/// Ignite client.
+[SuppressMessage("Reliability", "CA2000:Dispose objects before losing 
scope", Justification = "Pooled.")]
+public async ValueTask GetClientAsync()
+{
+ObjectDisposedException.ThrowIf(IsDisposed, this);
+
+int index = Interlocked.Increment(ref _clientIndex) % _clients.Length;
+
+IgniteClientInternal? client = _clients[index];
+if (client is { IsDisposed: false })

Review Comment:
   Agree. Changed it to return `IIgnite` instead, which is not disposable.



-- 
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-unsubscr...@ignite.apache.org

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