Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Minor improvements to Service Fabric providers #3250

Merged
merged 4 commits into from
Aug 1, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions src/OrleansServiceFabricUtils/FabricGatewayProvider.cs
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,8 @@ internal class FabricGatewayProvider : IGatewayListProvider, IGatewayListObserva
public FabricGatewayProvider(IFabricServiceSiloResolver siloResolver)
{
this.fabricServiceSiloResolver = siloResolver;
this.refreshPeriod = TimeSpan.FromSeconds(30);
this.MaxStaleness = TimeSpan.FromSeconds(this.refreshPeriod.TotalSeconds * 2);
this.refreshPeriod = TimeSpan.FromSeconds(5);
this.MaxStaleness = this.refreshPeriod;
}

/// <inheritdoc />
Expand All @@ -67,14 +67,16 @@ public async Task InitializeGatewayListProvider(ClientConfiguration clientConfig
/// <inheritdoc />
public bool SubscribeToGatewayNotificationEvents(IGatewayListListener subscriber)
{
this.log.Verbose($"Subscribing {subscriber} to gateway notification events.");
this.subscribers.TryAdd(subscriber, subscriber);
return true;
}

/// <inheritdoc />
public bool UnSubscribeFromGatewayNotificationEvents(IGatewayListListener listener)
public bool UnSubscribeFromGatewayNotificationEvents(IGatewayListListener subscriber)
{
this.subscribers.TryRemove(listener, out listener);
this.log.Verbose($"Unsubscribing {subscriber} from gateway notification events.");
this.subscribers.TryRemove(subscriber, out subscriber);
return true;
}

Expand Down
2 changes: 1 addition & 1 deletion src/OrleansServiceFabricUtils/FabricMembershipOracle.cs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ internal class FabricMembershipOracle : IMembershipOracle, IFabricServiceStatusL

private readonly AutoResetEvent notificationEvent = new AutoResetEvent(false);
private readonly BlockingCollection<StatusChangeNotification> notifications = new BlockingCollection<StatusChangeNotification>();
private readonly TimeSpan refreshPeriod = TimeSpan.FromSeconds(30);
private readonly TimeSpan refreshPeriod = TimeSpan.FromSeconds(5);
private readonly ILocalSiloDetails localSiloDetails;
private readonly GlobalConfiguration globalConfig;
private readonly IFabricServiceSiloResolver fabricServiceSiloResolver;
Expand Down
11 changes: 2 additions & 9 deletions src/OrleansServiceFabricUtils/FabricServiceSiloResolver.cs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
using System;
using System;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Fabric;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
Expand Down Expand Up @@ -132,14 +133,6 @@ private void OnPartitionChange(long handlerId, FabricPartitionResolutionChange a
if (!existing.IsSamePartitionAs(updated.Partition)) continue;
found = true;

if (updated.Partition.IsOlderThan(existing))
{
this.log.Info($"Update for partition {updated} is superseded by existing version.");

// Do not update the partition if the exiting one has a newer version than the update.
break;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not an expert on service fabric. But would removing this post the risk of older partition overwrites newer partition? Yes the older partition would mostly be corrected by newer partition eventually. But this brought unnecessary handling on partitionChange, which can be avoided, right? or is this pre-mature optimization?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really, I was just finding that these checks really don't add value. In the worst case - that there's some race and updates are out of order, we have stale information for 1 polling cycle (5 seconds).

}

// Update the partition.
this.silos[i] = updated;
this.NotifySubscribers();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,7 @@ internal interface IResolvedServicePartition
Guid Id { get; }

ServicePartitionKind Kind { get; }

bool IsOlderThan(IResolvedServicePartition other);


bool IsSamePartitionAs(IResolvedServicePartition other);
}
}
8 changes: 0 additions & 8 deletions src/OrleansServiceFabricUtils/Utilities/FabricQueryManager.cs
Original file line number Diff line number Diff line change
Expand Up @@ -155,14 +155,6 @@ public ResolvedServicePartitionWrapper(ResolvedServicePartition partition)

public ServicePartitionKind Kind => this.Partition.Info.Kind;

public bool IsOlderThan(IResolvedServicePartition other)
{
var otherWrapper = other as ResolvedServicePartitionWrapper;
if (otherWrapper == null) return false;

return this.Partition.IsOlderThan(otherWrapper.Partition);
}

public bool IsSamePartitionAs(IResolvedServicePartition other)
{
var otherWrapper = other as ResolvedServicePartitionWrapper;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,10 @@ internal static class ResolvedServicePartitionExtensions
/// <returns>The active endpoints published by the specified partition.</returns>
public static List<FabricSiloInfo> GetPartitionEndpoints(this ResolvedServicePartition partition)
{
var results = new List<FabricSiloInfo>(partition.Endpoints.Count);
foreach (var silo in partition.Endpoints)
var resolvedServiceEndpoints = partition.Endpoints;
if (resolvedServiceEndpoints == null) return new List<FabricSiloInfo>();
var results = new List<FabricSiloInfo>(resolvedServiceEndpoints.Count);
foreach (var silo in resolvedServiceEndpoints)
{
// Find the primary endpoint. If this is a stateless service, find any endpoint.
if (silo.Role != ServiceEndpointRole.Stateless && silo.Role != ServiceEndpointRole.StatefulPrimary) continue;
Expand All @@ -30,7 +32,7 @@ public static List<FabricSiloInfo> GetPartitionEndpoints(this ResolvedServicePar
}

/// <summary>
/// Respresents endpoints returned from <see cref="ResolvedServiceEndpoint.Address"/> in JSON form.
/// Represents endpoints returned from <see cref="ResolvedServiceEndpoint.Address"/> in JSON form.
/// </summary>
internal class ServicePartitionEndpoints
{
Expand Down
13 changes: 0 additions & 13 deletions src/OrleansServiceFabricUtils/Utilities/ServiceFabricExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -6,19 +6,6 @@ namespace Microsoft.Orleans.ServiceFabric.Utilities
{
internal static class ServiceFabricExtensions
{
/// <summary>
/// Returns a value indicating whether or not <paramref name="left"/> is older than <paramref name="right"/>.
/// </summary>
/// <param name="left">One resolved partition.</param>
/// <param name="right">The other resolved partition.</param>
/// <returns>
/// <see langword="true"/> if <paramref name="left"/> is older than <paramref name="right"/>, <see langword="false"/> otherwise.
/// </returns>
public static bool IsOlderThan(this ResolvedServicePartition left, ResolvedServicePartition right)
{
return left.Info.Id == right.Info.Id && left.CompareVersion(right) < 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can this be justleft.CompareVersion(right) < 0;? so that we removed the unnecessary check and also compared the version, so that FabricServiceSiloResolver.OnPartitionChange won't be processing older partition.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can, but the check doesn't provide value

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, that wouldn't work - because the version would be reset when the partition ids change (and anyhow the two versions wouldn't be related to each other since they're for different physical partitions)

}

/// <summary>
/// Returns a value indicating whether or not <paramref name="left"/> is the same partition as <paramref name="right"/>.
/// </summary>
Expand Down