You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "ptupitsyn (via GitHub)" <gi...@apache.org> on 2024/11/20 15:45:38 UTC
[PR] IGNITE-23703 .NET: Add client pool [ignite-3]
ptupitsyn opened a new pull request, #4763:
URL: https://github.com/apache/ignite-3/pull/4763
(no comment)
--
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
Re: [PR] IGNITE-23703 .NET: Add client pool [ignite-3]
Posted by "ptupitsyn (via GitHub)" <gi...@apache.org>.
ptupitsyn commented on code in PR #4763:
URL: https://github.com/apache/ignite-3/pull/4763#discussion_r1853405687
##########
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;
+
+/// <summary>
+/// Ignite client pool. Thread safe.
+/// <para>
+/// This pool creates up to <see cref="IgniteClientPoolConfiguration.Size"/> Ignite clients and returns them in a round-robin fashion.
+/// Ignite clients are thread safe, so there is no rent/return semantics.
+/// </para>
+/// <example>
+/// Register as a singleton in DI container:
+/// <code>
+/// builder.Services.AddSingleton(_ => new IgniteClientPool(
+/// new IgniteClientPoolConfiguration
+/// {
+/// Size = 3,
+/// ClientConfiguration = new("localhost"),
+/// }));
+/// </code>
+/// Invoke from a controller:
+/// <code>
+/// public async Task<IActionResult> Index([FromServices] IgniteClientPool ignite)
+/// {
+/// var client = await ignite.GetClientAsync();
+/// var tables = await client.Tables.GetTablesAsync();
+/// return Ok(tables);
+/// }
+/// </code>
+/// </example>
+/// </summary>
+public sealed class IgniteClientPool : IDisposable
+{
+ private readonly IgniteClientInternal?[] _clients;
+
+ private readonly SemaphoreSlim _clientsLock = new(1);
+
+ private int _disposed;
+
+ private int _clientIndex;
+
+ /// <summary>
+ /// Initializes a new instance of the <see cref="IgniteClientPool"/> class.
+ /// </summary>
+ /// <param name="configuration">Configuration.</param>
+ 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];
+ }
+
+ /// <summary>
+ /// Gets the configuration.
+ /// </summary>
+ public IgniteClientPoolConfiguration Configuration { get; }
+
+ /// <summary>
+ /// Gets a value indicating whether the pool is disposed.
+ /// </summary>
+ public bool IsDisposed => Interlocked.CompareExchange(ref _disposed, 0, 0) == 1;
+
+ /// <summary>
+ /// Gets an Ignite client from the pool. Creates a new one if necessary.
+ /// Performs round-robin balancing across pooled instances.
+ /// <para />
+ /// NOTE: Do not dispose the client instance returned by this method, it is managed by the pool.
+ /// </summary>
+ /// <returns>Ignite client.</returns>
+ [SuppressMessage("Reliability", "CA2000:Dispose objects before losing scope", Justification = "Pooled.")]
+ public async ValueTask<IIgniteClient> GetClientAsync()
+ {
+ ObjectDisposedException.ThrowIf(IsDisposed, this);
+
+ int index = Interlocked.Increment(ref _clientIndex) % _clients.Length;
+
+ IgniteClientInternal? client = _clients[index];
+ if (client is { IsDisposed: false })
Review Comment:
Please elaborate.
Yes, I understand that we can return a disposed client because some other thread closes it after this check, but there is no way to improve this situation, right?
--
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
Re: [PR] IGNITE-23703 .NET: Add IgniteClientGroup [ignite-3]
Posted by "gurustron (via GitHub)" <gi...@apache.org>.
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23703 .NET: Add IgniteClientGroup [ignite-3]
Posted by "ptupitsyn (via GitHub)" <gi...@apache.org>.
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;
+
+/// <summary>
+/// Ignite client pool. Thread safe.
+/// <para>
+/// This pool creates up to <see cref="IgniteClientPoolConfiguration.Size"/> Ignite clients and returns them in a round-robin fashion.
+/// Ignite clients are thread safe, so there is no rent/return semantics.
+/// </para>
+/// <example>
+/// Register as a singleton in DI container:
+/// <code>
+/// builder.Services.AddSingleton(_ => new IgniteClientPool(
+/// new IgniteClientPoolConfiguration
+/// {
+/// Size = 3,
+/// ClientConfiguration = new("localhost"),
+/// }));
+/// </code>
+/// Invoke from a controller:
+/// <code>
+/// public async Task<IActionResult> Index([FromServices] IgniteClientPool ignite)
+/// {
+/// var client = await ignite.GetClientAsync();
+/// var tables = await client.Tables.GetTablesAsync();
+/// return Ok(tables);
+/// }
+/// </code>
+/// </example>
+/// </summary>
+public sealed class IgniteClientPool : IDisposable
+{
+ private readonly IgniteClientInternal?[] _clients;
+
+ private readonly SemaphoreSlim _clientsLock = new(1);
+
+ private int _disposed;
+
+ private int _clientIndex;
+
+ /// <summary>
+ /// Initializes a new instance of the <see cref="IgniteClientPool"/> class.
+ /// </summary>
+ /// <param name="configuration">Configuration.</param>
+ 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];
+ }
+
+ /// <summary>
+ /// Gets the configuration.
+ /// </summary>
+ public IgniteClientPoolConfiguration Configuration { get; }
+
+ /// <summary>
+ /// Gets a value indicating whether the pool is disposed.
+ /// </summary>
+ public bool IsDisposed => Interlocked.CompareExchange(ref _disposed, 0, 0) == 1;
+
+ /// <summary>
+ /// Gets an Ignite client from the pool. Creates a new one if necessary.
+ /// Performs round-robin balancing across pooled instances.
+ /// <para />
+ /// NOTE: Do not dispose the client instance returned by this method, it is managed by the pool.
+ /// </summary>
+ /// <returns>Ignite client.</returns>
+ [SuppressMessage("Reliability", "CA2000:Dispose objects before losing scope", Justification = "Pooled.")]
+ public async ValueTask<IIgniteClient> 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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23703 .NET: Add client pool [ignite-3]
Posted by "ptupitsyn (via GitHub)" <gi...@apache.org>.
ptupitsyn commented on PR #4763:
URL: https://github.com/apache/ignite-3/pull/4763#issuecomment-2493073151
> Maybe follow the factory pattern
In this case, as a user, I would assume the following:
* Factory returns a new object every time
* The object "belongs" to this caller and no other caller sees it
* I should release the resources when done
None of that is correct.
> Pool without returning to the pool is a bit confusing TBH
I'm not sure, that's why I asked for your review :)
Certainly less confusing than the factory that reuses objects.
Maybe we should use a different name, like `IgniteClientGroup`?
--
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
Re: [PR] IGNITE-23703 .NET: Add client pool [ignite-3]
Posted by "gurustron (via GitHub)" <gi...@apache.org>.
gurustron commented on PR #4763:
URL: https://github.com/apache/ignite-3/pull/4763#issuecomment-2491445505
Maybe move the following:
```csharp
builder.Services.AddSingleton<IgniteClientPool>(_ => new IgniteClientPool(
```
Into a specific `AddIgniteClientPool` method accepting setup action.
--
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
Re: [PR] IGNITE-23703 .NET: Add client pool [ignite-3]
Posted by "gurustron (via GitHub)" <gi...@apache.org>.
gurustron commented on code in PR #4763:
URL: https://github.com/apache/ignite-3/pull/4763#discussion_r1852305889
##########
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;
+
+/// <summary>
+/// Ignite client pool. Thread safe.
+/// <para>
+/// This pool creates up to <see cref="IgniteClientPoolConfiguration.Size"/> Ignite clients and returns them in a round-robin fashion.
+/// Ignite clients are thread safe, so there is no rent/return semantics.
+/// </para>
+/// <example>
+/// Register as a singleton in DI container:
+/// <code>
+/// builder.Services.AddSingleton(_ => new IgniteClientPool(
+/// new IgniteClientPoolConfiguration
+/// {
+/// Size = 3,
+/// ClientConfiguration = new("localhost"),
+/// }));
+/// </code>
+/// Invoke from a controller:
+/// <code>
+/// public async Task<IActionResult> Index([FromServices] IgniteClientPool ignite)
+/// {
+/// var client = await ignite.GetClientAsync();
+/// var tables = await client.Tables.GetTablesAsync();
+/// return Ok(tables);
+/// }
+/// </code>
+/// </example>
+/// </summary>
+public sealed class IgniteClientPool : IDisposable
+{
+ private readonly IgniteClientInternal?[] _clients;
+
+ private readonly SemaphoreSlim _clientsLock = new(1);
+
+ private int _disposed;
+
+ private int _clientIndex;
+
+ /// <summary>
+ /// Initializes a new instance of the <see cref="IgniteClientPool"/> class.
+ /// </summary>
+ /// <param name="configuration">Configuration.</param>
+ 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];
+ }
+
+ /// <summary>
+ /// Gets the configuration.
+ /// </summary>
+ public IgniteClientPoolConfiguration Configuration { get; }
+
+ /// <summary>
+ /// Gets a value indicating whether the pool is disposed.
+ /// </summary>
+ public bool IsDisposed => Interlocked.CompareExchange(ref _disposed, 0, 0) == 1;
+
+ /// <summary>
+ /// Gets an Ignite client from the pool. Creates a new one if necessary.
+ /// Performs round-robin balancing across pooled instances.
+ /// <para />
+ /// NOTE: Do not dispose the client instance returned by this method, it is managed by the pool.
+ /// </summary>
+ /// <returns>Ignite client.</returns>
+ [SuppressMessage("Reliability", "CA2000:Dispose objects before losing scope", Justification = "Pooled.")]
+ public async ValueTask<IIgniteClient> GetClientAsync()
+ {
+ ObjectDisposedException.ThrowIf(IsDisposed, this);
+
+ int index = Interlocked.Increment(ref _clientIndex) % _clients.Length;
+
+ IgniteClientInternal? client = _clients[index];
+ if (client is { IsDisposed: false })
Review Comment:
not thread safe =)))
--
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
Re: [PR] IGNITE-23703 .NET: Add client pool [ignite-3]
Posted by "gurustron (via GitHub)" <gi...@apache.org>.
gurustron commented on code in PR #4763:
URL: https://github.com/apache/ignite-3/pull/4763#discussion_r1853432517
##########
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;
+
+/// <summary>
+/// Ignite client pool. Thread safe.
+/// <para>
+/// This pool creates up to <see cref="IgniteClientPoolConfiguration.Size"/> Ignite clients and returns them in a round-robin fashion.
+/// Ignite clients are thread safe, so there is no rent/return semantics.
+/// </para>
+/// <example>
+/// Register as a singleton in DI container:
+/// <code>
+/// builder.Services.AddSingleton(_ => new IgniteClientPool(
+/// new IgniteClientPoolConfiguration
+/// {
+/// Size = 3,
+/// ClientConfiguration = new("localhost"),
+/// }));
+/// </code>
+/// Invoke from a controller:
+/// <code>
+/// public async Task<IActionResult> Index([FromServices] IgniteClientPool ignite)
+/// {
+/// var client = await ignite.GetClientAsync();
+/// var tables = await client.Tables.GetTablesAsync();
+/// return Ok(tables);
+/// }
+/// </code>
+/// </example>
+/// </summary>
+public sealed class IgniteClientPool : IDisposable
+{
+ private readonly IgniteClientInternal?[] _clients;
+
+ private readonly SemaphoreSlim _clientsLock = new(1);
+
+ private int _disposed;
+
+ private int _clientIndex;
+
+ /// <summary>
+ /// Initializes a new instance of the <see cref="IgniteClientPool"/> class.
+ /// </summary>
+ /// <param name="configuration">Configuration.</param>
+ 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];
+ }
+
+ /// <summary>
+ /// Gets the configuration.
+ /// </summary>
+ public IgniteClientPoolConfiguration Configuration { get; }
+
+ /// <summary>
+ /// Gets a value indicating whether the pool is disposed.
+ /// </summary>
+ public bool IsDisposed => Interlocked.CompareExchange(ref _disposed, 0, 0) == 1;
+
+ /// <summary>
+ /// Gets an Ignite client from the pool. Creates a new one if necessary.
+ /// Performs round-robin balancing across pooled instances.
+ /// <para />
+ /// NOTE: Do not dispose the client instance returned by this method, it is managed by the pool.
+ /// </summary>
+ /// <returns>Ignite client.</returns>
+ [SuppressMessage("Reliability", "CA2000:Dispose objects before losing scope", Justification = "Pooled.")]
+ public async ValueTask<IIgniteClient> GetClientAsync()
+ {
+ ObjectDisposedException.ThrowIf(IsDisposed, this);
+
+ int index = Interlocked.Increment(ref _clientIndex) % _clients.Length;
+
+ IgniteClientInternal? client = _clients[index];
+ if (client is { IsDisposed: false })
Review Comment:
My idea was that `GetClientAsync` should return something which is not disposable, so the full control is on the `IgniteClientPool` side, then the approach with `_clientsLock` is the way to go.
--
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
Re: [PR] IGNITE-23703 .NET: Add client pool [ignite-3]
Posted by "ptupitsyn (via GitHub)" <gi...@apache.org>.
ptupitsyn commented on PR #4763:
URL: https://github.com/apache/ignite-3/pull/4763#issuecomment-2493075297
@gurustron
> Into a specific AddIgniteClientPool method accepting setup action.
Yes, we will do that in [IGNITE-23651 .NET: Thin 3.0: Add ServiceCollection extensions for DI integration](https://issues.apache.org/jira/browse/IGNITE-23651)
--
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
Re: [PR] IGNITE-23703 .NET: Add client pool [ignite-3]
Posted by "gurustron (via GitHub)" <gi...@apache.org>.
gurustron commented on PR #4763:
URL: https://github.com/apache/ignite-3/pull/4763#issuecomment-2491466968
Maybe follow the factory pattern - as EF Core does with [`IDbContextFactory`](https://learn.microsoft.com/en-us/ef/core/dbcontext-configuration/#use-a-dbcontext-factory)? Pool without returning to the pool is a bit confusing TBH.
--
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
Re: [PR] IGNITE-23703 .NET: Add IgniteClientGroup [ignite-3]
Posted by "ptupitsyn (via GitHub)" <gi...@apache.org>.
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org