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

[Lease.Azure] Async task and options code cleanup #1256

Merged
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ public void HostingExtension1Test()
.Should().NotBeNull();
}

[Fact(DisplayName = "Hosting Action<Setup> extension should add Setup class and default hocon settings")]
[Fact(DisplayName = "Hosting Action<Options> extension should add default hocon settings")]
public void HostingExtension2Test()
{
var builder = new AkkaConfigurationBuilder(new ServiceCollection(), "test");
Expand All @@ -37,15 +37,12 @@ public void HostingExtension2Test()
});

builder.Configuration.HasValue.Should().BeTrue();
builder.Configuration.Value.GetConfig("akka.coordination.lease.azure")
.Should().NotBeNull();
var setup = ExtractSetup(builder);
setup.Should().NotBeNull();
// !: null checked above
setup!.ContainerName.Should().Be("underTest");
var config = builder.Configuration.Value.GetConfig("akka.coordination.lease.azure");
config.Should().NotBeNull();
config.GetString("container-name").Should().Be("underTest");
}

[Fact(DisplayName = "Hosting Setup extension should add Setup class and default hocon settings")]
[Fact(DisplayName = "Hosting options extension should add default hocon settings")]
public void HostingExtension3Test()
{
var builder = new AkkaConfigurationBuilder(new ServiceCollection(), "test");
Expand All @@ -56,17 +53,9 @@ public void HostingExtension3Test()
});

builder.Configuration.HasValue.Should().BeTrue();
builder.Configuration.Value.GetConfig("akka.coordination.lease.azure")
.Should().NotBeNull();
var setup = ExtractSetup(builder);
setup.Should().NotBeNull();
// !: null checked above
setup!.ContainerName.Should().Be("underTest");
}

private static AzureLeaseSetup? ExtractSetup(AkkaConfigurationBuilder builder)
{
return builder.Setups.FirstOrDefault(s => s is AzureLeaseSetup) as AzureLeaseSetup;
var config = builder.Configuration.Value.GetConfig("akka.coordination.lease.azure");
config.Should().NotBeNull();
config.GetString("container-name").Should().Be("underTest");
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
// -----------------------------------------------------------------------
// <copyright file="AzureLeaseSpec.cs" company="Akka.NET Project">
// Copyright (C) 2013-2023 .NET Foundation <https://github.com/akkadotnet/akka.net>
// </copyright>
// -----------------------------------------------------------------------

using System;
using System.Threading.Tasks;
using Akka.Actor;
using Akka.Configuration;
using Xunit;
using Xunit.Abstractions;
using FluentAssertions;

namespace Akka.Coordination.Azure.Tests;

public class AzureLeaseSpec: TestKit.Xunit2.TestKit, IAsyncLifetime
{
private const string LeaseName = "lease";
private const string OwnerName = "owner1";

private static Config Config()
=> ConfigurationFactory.ParseString(@"
akka.loglevel=DEBUG
akka.stdout-loglevel=DEBUG
akka.actor.debug.fsm=true
akka.remote.dot-netty.tcp.port = 0
akka.coordination.lease.azure.connection-string = ""UseDevelopmentStorage=true""
")
.WithFallback(AzureLease.DefaultConfiguration);

private readonly Lease _lease;

public AzureLeaseSpec(ITestOutputHelper helper): base(Config(), nameof(AzureLeaseSpec), helper)
{
_lease = LeaseProvider.Get(Sys).GetLease(LeaseName, "akka.coordination.lease.azure", OwnerName);
}

[Fact(DisplayName = "Releasing non-acquired lease should not throw an exception")]
public void NonAcquiredReleaseTest()
{
var probe = CreateTestProbe();
var _ = _lease.Release().ContinueWith(r =>
{
r.IsFaulted.Should().BeTrue();
r.Exception.Should().NotBeNull();
var exception = r.Exception!.InnerException;
exception.Should().NotBeNull();
exception.Should().BeOfType<LeaseException>();
exception!.Message.Should().Be("Tried to release a lease that is not acquired");
probe.Tell(Done.Instance);
});

probe.ExpectMsg<Done>();
}

public async Task InitializeAsync()
{
await Util.Cleanup("UseDevelopmentStorage=true");
}

public Task DisposeAsync()
{
return Task.CompletedTask;
}
}
42 changes: 17 additions & 25 deletions src/coordination/azure/Akka.Coordination.Azure/AzureLease.cs
Original file line number Diff line number Diff line change
Expand Up @@ -85,23 +85,19 @@ public AzureLease(ExtendedActorSystem system, AtomicBoolean leaseTaken, LeaseSet
public override bool CheckLease()
=> _leaseTaken.Value;

public override Task<bool> Release()
public override async Task<bool> Release()
{
// replace with transform once 2.11 dropped
try
{
if(_log.IsDebugEnabled)
_log.Debug("Releasing lease");
return _leaseActor.Ask(LeaseActor.Release.Instance, _timeout)
.ContinueWith(t =>
{
return t.Result switch
{
LeaseActor.LeaseReleased _ => true,
LeaseActor.InvalidRequest req => throw new LeaseException(req.Reason),
_ => false
};
});
var result = await _leaseActor.Ask(LeaseActor.Release.Instance, _timeout);
Aaronontheweb marked this conversation as resolved.
Show resolved Hide resolved
return result switch
{
LeaseActor.LeaseReleased _ => true,
LeaseActor.InvalidRequest req => throw new LeaseException(req.Reason),
_ => throw new LeaseException($"Unexpected response type: {result.GetType()}")
};
}
catch (AskTimeoutException)
{
Expand All @@ -113,24 +109,20 @@ public override Task<bool> Release()
public override Task<bool> Acquire()
=> Acquire(null);

public override Task<bool> Acquire(Action<Exception?>? leaseLostCallback)
public override async Task<bool> Acquire(Action<Exception?>? leaseLostCallback)
{
// replace with transform once 2.11 dropped
try
{
if(_log.IsDebugEnabled)
_log.Debug("Acquiring lease");
return _leaseActor.Ask(new LeaseActor.Acquire(leaseLostCallback), _timeout)
.ContinueWith(t =>
{
return t.Result switch
{
LeaseActor.LeaseAcquired _ => true,
LeaseActor.LeaseTaken _ => false,
LeaseActor.InvalidRequest req => throw new LeaseException(req.Reason),
_ => false
};
});
var result = await _leaseActor.Ask(new LeaseActor.Acquire(leaseLostCallback), _timeout);
Aaronontheweb marked this conversation as resolved.
Show resolved Hide resolved
return result switch
{
LeaseActor.LeaseAcquired _ => true,
LeaseActor.LeaseTaken _ => false,
LeaseActor.InvalidRequest req => throw new LeaseException(req.Reason),
_ => throw new LeaseException($"Unexpected response type: {result.GetType()}")
};
}
catch (AskTimeoutException)
{
Expand Down
Original file line number Diff line number Diff line change
@@ -1,14 +1,12 @@
// -----------------------------------------------------------------------
// <copyright file="AzureLeaseOption.cs" company="Akka.NET Project">
// Copyright (C) 2009-2022 Lightbend Inc. <http://www.lightbend.com>
// Copyright (C) 2013-2022 .NET Foundation <https://github.com/akkadotnet/akka.net>
// </copyright>
// -----------------------------------------------------------------------

using System;
using System.Text;
using Akka.Actor.Setup;
using Akka.Cluster.Hosting.SBR;
using Akka.Configuration;
using Akka.Hosting;
using Akka.Hosting.Coordination;
Expand Down Expand Up @@ -51,20 +49,20 @@ public override void Apply(AkkaConfigurationBuilder builder, Setup? s = null)
sb.AppendLine($"lease-operation-timeout = {LeaseOperationTimeout.ToHocon()}");
sb.AppendLine("}");

builder.AddHocon(sb.ToString(), HoconAddMode.Prepend);

if(AzureCredential is null && ServiceEndpoint is null)
return;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Code cleanup, shortcut Apply to not inject a Setup class if there is no need for it


if ((ServiceEndpoint is { } && AzureCredential is null) ||
(ServiceEndpoint is null && AzureCredential is { }))
throw new ConfigurationException("To use AzureCredential, both AzureCredential and ServiceEndpoint need to be populated.");

builder.AddHocon(sb.ToString(), HoconAddMode.Prepend);

var setup = new AzureLeaseSetup
{
AzureCredential = AzureCredential,
ServiceEndpoint = ServiceEndpoint,
BlobClientOptions = BlobClientOptions,
ApiServiceRequestTimeout = ApiServiceRequestTimeout,
ConnectionString = ConnectionString,
ContainerName = ContainerName
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Code cleanup, remove non-needed information from Setup class

};

builder.AddSetup(setup);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,7 @@ public async Task<LeaseResource> ReadOrCreateLeaseResource(string name)
{
switch ((HttpStatusCode)e.Status)
{
case HttpStatusCode.PreconditionFailed:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bug fix, we did not use any pre-condition in our code but the driver implicitly added a pre-condition internally, need to capture this exception.

Copy link
Member

Choose a reason for hiding this comment

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

Nice catch

case HttpStatusCode.Conflict:
_log.Debug(e,
"Creation of lease resource failed as already exists. Will attempt to read again");
Expand Down