#1634 #1487 #1329 #1304 #1294 #793 Consul polling of services: enhancements and fix errors #1670
Conversation
|
Nice PR, Guillaume! 😸 You forgot to request my review. 😉 |
raman-m
changed the title
|
@raman-m I can't add you as a reviewer... |
There was a problem hiding this comment.
Everything is fine in this PR except the SemaphoreSlim class usage.
According to the Remarks for SemaphoreSlim:
Semaphores are of two types: local semaphores and named system semaphores. Local semaphores are local to an application, system semaphores are visible throughout the operating system and are suitable for inter-process synchronization.
I see that we need not system semaphore and you just use local semaphore functionality in this PR. So, and I believe we can switch to classic lock-operator to manage access for _services collection.
Another option is just using of any thread-safe collections.
test/Ocelot.UnitTests/Consul/ConsulFileConfigurationRepositoryTests.cs
Outdated
Show resolved
Hide resolved
raman-m
added
the
waiting
|
@raman-m Just checked the changes (with lock) in a docker container. It works |
What do you mean? Maybe in the future an editorconfig file would help? |
Interesting Visual Studio feature, but not now! I meant the improvements in your code to read. Not in entire project! 🤣 |
Ok, but, in general, it would be a nice addition. |
There was a problem hiding this comment.
+ Approved once againI've made some code improvements with a little refactoring, plus updated unit tests.
I'm just thinking...
Should we update the documentation or not?
There are the same Consul and PollConsul types of ServiceDiscoveryProvider(s).
Maybe it makes sense to emphasize that Consul is default type provider even if type is not specified (in case of .AddConsul())...
What do you think?
raman-m
added
accepted
|
@raman-m So, I thought we could do the same with the kubernetes provider factory, like Eureka and Consul... |
Yes, we could! |
|
@raman-m Hello, it's done... |
|
@raman-m @RaynaldM I might have a solution to generalize the "polling" for Consul, Kubernetes and Eureka... Should I push it, or would you like to merge first and then open a new PR? using Ocelot.Logging;
using Ocelot.ServiceDiscovery.Providers;
namespace Ocelot.Polling;
public class PollingServicesManager<T, TU>
where T : class, IServiceDiscoveryProvider
where TU : ServicePollingHandler<T>
{
private readonly object _lockObject = new();
private readonly List<ServicePollingHandler<T>> _serviceDiscoveryProviders = new();
public ServicePollingHandler<T> GetServicePollingHandler(T baseProvider, string serviceName, int pollingInterval,
IOcelotLoggerFactory factory)
{
lock (_lockObject)
{
var discoveryProvider = _serviceDiscoveryProviders.FirstOrDefault(x => x.ServiceName == serviceName);
if (discoveryProvider != null)
{
return discoveryProvider;
}
discoveryProvider =
(TU)Activator.CreateInstance(typeof(TU), baseProvider, pollingInterval, serviceName, factory);
_serviceDiscoveryProviders.Add(discoveryProvider);
return (TU)discoveryProvider;
}
}
}using Ocelot.Logging;
using Ocelot.ServiceDiscovery.Providers;
using Ocelot.Values;
namespace Ocelot.Polling;
public abstract class ServicePollingHandler<T> : IServiceDiscoveryProvider
where T : class, IServiceDiscoveryProvider
{
private readonly T _baseProvider;
private readonly object _lockObject = new();
private readonly IOcelotLogger _logger;
private readonly int _pollingInterval;
private DateTime _lastUpdateTime;
private List<Service> _services;
protected ServicePollingHandler(T baseProvider, int pollingInterval, string serviceName,
IOcelotLoggerFactory factory)
{
_logger = factory.CreateLogger<ServicePollingHandler<T>>();
_pollingInterval = pollingInterval;
// Initialize by DateTime.MinValue as lowest value.
// Polling will occur immediately during the first call
_lastUpdateTime = DateTime.MinValue;
_services = new List<Service>();
ServiceName = serviceName;
_baseProvider = baseProvider;
}
public string ServiceName { get; protected set; }
public Task<List<Service>> Get()
{
lock (_lockObject)
{
var refreshTime = _lastUpdateTime.AddMilliseconds(_pollingInterval);
// Check if any services available
if (refreshTime >= DateTime.UtcNow && _services.Any())
{
return Task.FromResult(_services);
}
_logger.LogInformation($"Retrieving new client information for service: {ServiceName}.");
_services = _baseProvider.Get().Result;
_lastUpdateTime = DateTime.UtcNow;
return Task.FromResult(_services);
}
}
}using Ocelot.Logging;
using Ocelot.Polling;
namespace Ocelot.Provider.Consul;
public class PollConsul : ServicePollingHandler<Consul>
{
public PollConsul(Consul baseProvider, int pollingInterval, string serviceName, IOcelotLoggerFactory factory) : base(baseProvider, pollingInterval, serviceName, factory)
{
}
}using Microsoft.Extensions.DependencyInjection;
using Ocelot.Configuration;
using Ocelot.Logging;
using Ocelot.Polling;
using Ocelot.ServiceDiscovery.Providers;
namespace Ocelot.Provider.Consul;
public static class ConsulProviderFactory
{
/// <summary>
/// String constant used for provider type definition.
/// </summary>
public const string PollConsul = nameof(Provider.Consul.PollConsul);
private static readonly PollingServicesManager<Consul, PollConsul> ServicesManager = new();
public static ServiceDiscoveryFinderDelegate Get { get; } = CreateProvider;
private static IServiceDiscoveryProvider CreateProvider(IServiceProvider provider,
ServiceProviderConfiguration config, DownstreamRoute route)
{
var factory = provider.GetService<IOcelotLoggerFactory>();
var consulFactory = provider.GetService<IConsulClientFactory>();
var consulRegistryConfiguration = new ConsulRegistryConfiguration(
config.Scheme, config.Host, config.Port, route.ServiceName, config.Token);
var consulProvider = new Consul(consulRegistryConfiguration, factory, consulFactory);
if (PollConsul.Equals(config.Type, StringComparison.OrdinalIgnoreCase))
{
return ServicesManager.GetServicePollingHandler(consulProvider, route.ServiceName, config.PollingInterval, factory);
}
return consulProvider;
}
} |
|
@ggnaegi commented on Sep 27, 1:38pm:
Thank you for your intention to improve the code quality! But not in this PR please which is related to Consul services problems. |
|
@ggnaegi |
I don't know, you said docs? |
|
@raman-m so, first merge to develop then update the docs? |
No! I believe you did everything for this PR if you cannot (don't know how) to review docs. I hope, this your PR will be updated & merged right today, because I've planned to work on it today. P.S. And, don't forget to sync fork, update develop of your forked repo. |
|
If you don't mind, I will make some improvements to the code... |
|
Please stop committing to the branch!
This explains clearly why I see StyleCop violations...
🆗 I agree on this! Going to create a next PR... after merging your one. |
@raman-m Yeah, Sorry, I'm using Resharper with my colleagues, I forgot that you are using StyleCop... :-) |
|
@raman-m Maybe you could add that to the readme? https://www.jetbrains.com/help/resharper/StyleCop_Styles.html#applying-settings-from-settings-stylecop-files |
|
@RaynaldM Could you look at this PR finally please, going to Files changes tab? |
| var result = Wait.WaitFor(3000).Until(() => | ||
| try | ||
| { | ||
| _result = provider.Get().GetAwaiter().GetResult(); |
There was a problem hiding this comment.
Why do we have so strange sentence?
Can it be simplified?
|
@ggnaegi Guillaume, congrats on merging it! 🎉 Happy Friday! 🥳 |
raman-m
added
accepted
Fixes #1634 #1487 #1329 #1304 #1294 #793
Proposing some improvements for Consul "polling"
Proposed Changes
PollConsuland in the factory.