zarusz / SlimMessageBus

Lightweight message bus interface for .NET (pub/sub and request-response) with transport plugins for popular message brokers.
Apache License 2.0
467 stars 78 forks source link

#263 outbox benchmark tweaks #265

Closed EtherZa closed 3 months ago

EtherZa commented 3 months ago

@zarusz I picked up a few issues when running the benchmarks, so threw together a couple fixes

I have picked up a few logical errors in #260 which is the reason for the delay.

zarusz commented 3 months ago

@EtherZa looks good to me. Let me know once you've done with the changes and resolved the comments.

I've re-run the tests, there seems to be some transient connection to Azure SQL.

zarusz commented 3 months ago

I believe there is a clash between the two Outbox tests and PRs that apply different sets of migrations. Perhaps the delete from the migrations table does not serve well.

I dropped the two tables from the Azure SQL instance to let them be recreated and migration table re-populated.

DROP TABLE [dbo].[__EFMigrationsHistory]
DROP TABLE [dbo].[IntTest_Customer]

The last error from the integration tests:

[xUnit.net 00:01:38.65]         [17:50:44 ERR] Microsoft.EntityFrameworkCore.Database.Command Failed executing DbCommand (161ms) [Parameters=[], CommandType='"Text"', CommandTimeout='30']
[xUnit.net 00:01:38.65]         ALTER TABLE [IntTest_Customer] ADD [UniqueId] nvarchar(max) NULL;
  Failed SlimMessageBus.Host.Outbox.DbContext.Test.OutboxTests.Given_CommandHandlerInTransaction_When_ExceptionThrownDuringHandlingRaisedAtTheEnd_Then_TransactionIsRolledBack_And_NoDataSaved_And_NoEventRaised(transactionType: SqlTransaction, busType: AzureSB, messageCount: 100) [17 s]
  Error Message:
   Microsoft.Data.SqlClient.SqlException : Column names in each table must be unique. Column name 'UniqueId' in table 'IntTest_Customer' is specified more than once.
EtherZa commented 3 months ago

@zarusz One other enhancement that you may want to consider is to use a unique topic name with AutoDeleteOnIdle for each test execution to ensure that no messages remain in the subscription from a previous failed test run.

ie. something like

            mbb.AddChildBus("ExternalBus", mbb =>
            {
                var topic = $"tests.outbox-benchmark/customer-events/{Guid.NewGuid():N}";
                mbb
                    .WithProviderServiceBus(cfg =>
                    {
                        cfg.ConnectionString = Secrets.Service.PopulateSecrets(configuration["Azure:ServiceBus"]);
                        cfg.PrefetchCount = 100; // fetch 100 messages at a time
                        cfg.TopologyProvisioning.CreateTopicOptions = opt => opt.AutoDeleteOnIdle = TimeSpan.FromMinutes(5);
                    })

I did not make the change now as it will require manually deleting the existing topic before it can be used as a root for this pattern.

sonarcloud[bot] commented 3 months ago

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
91.4% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

EtherZa commented 3 months ago

This is the best I can come up with for a single db implementation.

Use the same schema for both EF and outbox migrations; dropping it on every test. Any test that uses the same db context/schema will need to run in series unfortunately.

On the upside, it will create a completely clean copy of the db for every test run, as well as clean instance of the service bus (with a 5 min auto delete). Repeatable and isolated from other tests while using a single SQL database instance.

If you would like to go the full concurrency/parallel execution route, it might be worth looking at test containers. ASB/AEH can use different topics to achieve the same thing without containers being available.

zarusz commented 3 months ago

@EtherZa Well done with the changes - it looks better & clean!