From 98365bfd8b9ef110fa912bde47603d7840a3c43b Mon Sep 17 00:00:00 2001 From: Edward Garza Date: Wed, 3 May 2023 15:24:33 -0700 Subject: [PATCH 1/3] first draft of clearing out the client cache from a notification --- .../Client/IServicePartitionResolver.cs | 5 +++++ .../Client/ServicePartitionResolver.cs | 17 ++++++++++++++ .../Client/CommunicationClientCache.cs | 22 +++++++++++++++++-- .../Client/CommunicationClientFactoryBase.cs | 21 +++++++++++++++--- 4 files changed, 60 insertions(+), 5 deletions(-) diff --git a/src/Microsoft.ServiceFabric.Services/Client/IServicePartitionResolver.cs b/src/Microsoft.ServiceFabric.Services/Client/IServicePartitionResolver.cs index 97164bad..b2fdb7bb 100644 --- a/src/Microsoft.ServiceFabric.Services/Client/IServicePartitionResolver.cs +++ b/src/Microsoft.ServiceFabric.Services/Client/IServicePartitionResolver.cs @@ -19,6 +19,11 @@ namespace Microsoft.ServiceFabric.Services.Client /// public interface IServicePartitionResolver { + /// + /// Event handler that is fired when the service resolver receives a ServiceNotificationFilterMatched Event. + /// + event EventHandler ServiceNotificationEvent; + /// /// Resolves a partition of the specified service with specified back-off/retry settings on retry-able errors. /// diff --git a/src/Microsoft.ServiceFabric.Services/Client/ServicePartitionResolver.cs b/src/Microsoft.ServiceFabric.Services/Client/ServicePartitionResolver.cs index 21ebf0c6..cd15bb92 100644 --- a/src/Microsoft.ServiceFabric.Services/Client/ServicePartitionResolver.cs +++ b/src/Microsoft.ServiceFabric.Services/Client/ServicePartitionResolver.cs @@ -141,6 +141,11 @@ public ServicePartitionResolver( { } + /// + /// some documentation for stalling + /// + public event EventHandler ServiceNotificationEvent; + internal bool UseNotification { get; set; } /// @@ -675,12 +680,24 @@ private FabricClient GetClient() if (this.fabricClient == null) { this.fabricClient = this.createFabricClient.Invoke(); + if (this.UseNotification) + { + this.fabricClient.ServiceManager.ServiceNotificationFilterMatched += this.OnServiceNotificationFilterMatched; + } } return this.fabricClient; } } + private void OnServiceNotificationFilterMatched(object sender, EventArgs e) + { + if (e is FabricClient.ServiceManagementClient.ServiceNotificationEventArgs notificationargs) + { + this.ServiceNotificationEvent?.Invoke(sender, notificationargs); + } + } + private void ReportFaulted(FabricClient client) { lock (this.thisLock) diff --git a/src/Microsoft.ServiceFabric.Services/Communication/Client/CommunicationClientCache.cs b/src/Microsoft.ServiceFabric.Services/Communication/Client/CommunicationClientCache.cs index 78000799..8a619ee2 100644 --- a/src/Microsoft.ServiceFabric.Services/Communication/Client/CommunicationClientCache.cs +++ b/src/Microsoft.ServiceFabric.Services/Communication/Client/CommunicationClientCache.cs @@ -10,6 +10,7 @@ namespace Microsoft.ServiceFabric.Services.Communication.Client using System.Collections.Generic; using System.Fabric; using System.Globalization; + using System.Linq; using System.Threading; /// @@ -93,9 +94,12 @@ public bool TryGetClientCacheEntry( return partitionClientCache.TryGetClientCacheEntry(endpoint, listenerName, out cacheEntry); } - public void ClearClientCacheEntries(Guid partitionId) + public void ClearClientCacheEntries(Guid partitionId, ICollection validEndpoints) { - // Currently no op. To implement when we register for service change notifications. + if (this.clientCache.TryGetValue(partitionId, out PartitionClientCache clientCache)) + { + clientCache.TryRemoveClientCacheEntry(validEndpoints); + } } public void Dispose() @@ -240,6 +244,20 @@ public bool TryGetClientCacheEntry( out cacheEntry); } + public bool TryRemoveClientCacheEntry(ICollection validEndpoints) + { + bool removed = false; + foreach (var key in this.cache.Keys) + { + if (!validEndpoints.Contains(key.Endpoint)) + { + removed |= this.cache.TryRemove(key, out _); + } + } + + return removed; + } + // Cleaning up of cache entries needs to synchronize with the code that uses the cache entry and sets the // communicationClient inside the cache entry. So the removal of the cache-entry should set entry.IsInCache to false // and also remove the entry while holding the entry's semaphore, so that we don't purge a cache entry that has a valid diff --git a/src/Microsoft.ServiceFabric.Services/Communication/Client/CommunicationClientFactoryBase.cs b/src/Microsoft.ServiceFabric.Services/Communication/Client/CommunicationClientFactoryBase.cs index aa033ac3..beefd24d 100644 --- a/src/Microsoft.ServiceFabric.Services/Communication/Client/CommunicationClientFactoryBase.cs +++ b/src/Microsoft.ServiceFabric.Services/Communication/Client/CommunicationClientFactoryBase.cs @@ -41,15 +41,18 @@ public abstract class CommunicationClientFactoryBase : /// Optional ServicePartitionResolver /// Optional Custom exception handlers for the exceptions on the Client to Service communication channel /// Identifier to use in diagnostics traces from this component + /// Indicates that this factory should invalidated cached partition clients through notifications protected CommunicationClientFactoryBase( IServicePartitionResolver servicePartitionResolver = null, IEnumerable exceptionHandlers = null, - string traceId = null) + string traceId = null, + bool clearCachedClientsWithNotifications = false) : this( false, servicePartitionResolver, exceptionHandlers, - traceId) + traceId, + clearCachedClientsWithNotifications) { } @@ -60,16 +63,23 @@ protected CommunicationClientFactoryBase( /// Optional ServicePartitionResolver /// Optional Custom exception handlers for the exceptions on the Client to Service communication channel /// Identifier to use in diagnostics traces from this component + /// Indicates that this factory should invalidated cached partition clients through notifications protected CommunicationClientFactoryBase( bool fireConnectEvents, IServicePartitionResolver servicePartitionResolver = null, IEnumerable exceptionHandlers = null, - string traceId = null) + string traceId = null, + bool clearCachedClientsWithNotifications = false) { this.fireConnectEvents = fireConnectEvents; this.randomLock = new object(); this.traceId = traceId ?? Guid.NewGuid().ToString(); this.servicePartitionResolver = servicePartitionResolver ?? ServicePartitionResolver.GetDefault(); + if (clearCachedClientsWithNotifications) + { + this.servicePartitionResolver.ServiceNotificationEvent += this.ClearCache; + } + this.random = null; this.exceptionHandlers = new List(); if (exceptionHandlers != null) @@ -802,6 +812,11 @@ private bool ValidateLockedClientCacheEntry( return false; } + private void ClearCache(object sender, FabricClient.ServiceManagementClient.ServiceNotificationEventArgs eventArgs) + { + this.cache.ClearClientCacheEntries(eventArgs.Notification.PartitionId, eventArgs.Notification.Endpoints); + } + private int GenerateSeed() { var hashcode = this.GetHashCode(); From 91931891d8ca657b92fa370c7b2399f0ef6ea8a0 Mon Sep 17 00:00:00 2001 From: Edward Garza Date: Wed, 3 May 2023 15:32:36 -0700 Subject: [PATCH 2/3] update some documentation --- .../Communication/Client/CommunicationClientCache.cs | 11 +++++++++++ .../Client/CommunicationClientFactoryBase.cs | 4 ++-- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/src/Microsoft.ServiceFabric.Services/Communication/Client/CommunicationClientCache.cs b/src/Microsoft.ServiceFabric.Services/Communication/Client/CommunicationClientCache.cs index 8a619ee2..0ee80aba 100644 --- a/src/Microsoft.ServiceFabric.Services/Communication/Client/CommunicationClientCache.cs +++ b/src/Microsoft.ServiceFabric.Services/Communication/Client/CommunicationClientCache.cs @@ -94,6 +94,11 @@ public bool TryGetClientCacheEntry( return partitionClientCache.TryGetClientCacheEntry(endpoint, listenerName, out cacheEntry); } + /// + /// Clear the cache entries for a given replica that are not contains in validEndpoints + /// + /// Partition id of the replica set + /// Collection of endpoints that are valid for the replica set public void ClearClientCacheEntries(Guid partitionId, ICollection validEndpoints) { if (this.clientCache.TryGetValue(partitionId, out PartitionClientCache clientCache)) @@ -244,11 +249,17 @@ public bool TryGetClientCacheEntry( out cacheEntry); } + /// + /// Try to remove any cache items if the endpoint is not in the collection of valid endpoints + /// + /// Collection of valid endpoints + /// A boolean indicating if anything was removed from the cache public bool TryRemoveClientCacheEntry(ICollection validEndpoints) { bool removed = false; foreach (var key in this.cache.Keys) { + // if the cache for the replica set contains an endpoint that is not valid remove it if (!validEndpoints.Contains(key.Endpoint)) { removed |= this.cache.TryRemove(key, out _); diff --git a/src/Microsoft.ServiceFabric.Services/Communication/Client/CommunicationClientFactoryBase.cs b/src/Microsoft.ServiceFabric.Services/Communication/Client/CommunicationClientFactoryBase.cs index beefd24d..471eae60 100644 --- a/src/Microsoft.ServiceFabric.Services/Communication/Client/CommunicationClientFactoryBase.cs +++ b/src/Microsoft.ServiceFabric.Services/Communication/Client/CommunicationClientFactoryBase.cs @@ -41,7 +41,7 @@ public abstract class CommunicationClientFactoryBase : /// Optional ServicePartitionResolver /// Optional Custom exception handlers for the exceptions on the Client to Service communication channel /// Identifier to use in diagnostics traces from this component - /// Indicates that this factory should invalidated cached partition clients through notifications + /// Indicates that this factory should invalidate cached partition clients through notifications protected CommunicationClientFactoryBase( IServicePartitionResolver servicePartitionResolver = null, IEnumerable exceptionHandlers = null, @@ -63,7 +63,7 @@ protected CommunicationClientFactoryBase( /// Optional ServicePartitionResolver /// Optional Custom exception handlers for the exceptions on the Client to Service communication channel /// Identifier to use in diagnostics traces from this component - /// Indicates that this factory should invalidated cached partition clients through notifications + /// Indicates that this factory should invalidate cached partition clients through notifications protected CommunicationClientFactoryBase( bool fireConnectEvents, IServicePartitionResolver servicePartitionResolver = null, From e6ed12288787f668c59f423a89cbd21ef0876f80 Mon Sep 17 00:00:00 2001 From: Edward Garza Date: Wed, 3 May 2023 15:35:28 -0700 Subject: [PATCH 3/3] update documentation again --- .../Client/ServicePartitionResolver.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Microsoft.ServiceFabric.Services/Client/ServicePartitionResolver.cs b/src/Microsoft.ServiceFabric.Services/Client/ServicePartitionResolver.cs index cd15bb92..335894d0 100644 --- a/src/Microsoft.ServiceFabric.Services/Client/ServicePartitionResolver.cs +++ b/src/Microsoft.ServiceFabric.Services/Client/ServicePartitionResolver.cs @@ -142,7 +142,7 @@ public ServicePartitionResolver( } /// - /// some documentation for stalling + /// Event handler that is fired when the service resolver receives a ServiceNotificationFilterMatched Event. /// public event EventHandler ServiceNotificationEvent;