search

rebus-org / Rebus.SqlServer

:bus: Microsoft SQL Server transport and persistence for Rebus
https://mookid.dk/category/rebus
Other
43 stars 42 forks source link

Introduce Transport Options #53

Closed MrMDavidson closed 4 years ago

MrMDavidson commented 4 years ago

The existing many-different-ways of configuring the transports was getting difficult to maintain and missed some combinations. This PR introduces two new classes; SqlServerTransportOptions (for configuring SqlServerTransport) and SqlServerLeaseTransportOptions (for configuring SqlServerLeaseTransportOptions). Existing methods have been refactored, but left as is, and marked as obsolete for future removal.

Example uses;

config.Transport(
  c => {
    c.UseSqlServerInLeaseMode(
      new SqlServerServerLeaseTransportOptions(dbFactoryFunction) {
      InputQueueName = "InputMessages",
      EnsureTablesAreCreated = false,
      LeaseInterval = TimeSpan.FromSeconds(60)
  }
);

Or to create a one way client that uses the normal SQL Server transport;

config.Transport(
  c => {
    c.UseSqlServer(
      new SqlServerServerTransportOptions(dbConnectionProvider) {
      InputQueueName = null,
  }
);

I'm not sure if you want to promote one way mode to a "first class citizen" like it currently is... if so, I'd basically just have those overloads call through pre-configured. Eg.

public void UseSqlServerInLeaseModeAsOneWayClient(this StandardConfigurer<ITransport> configurer, SqlServerLeaseTransportOptions transportOptions) {
  transportOptions.InputQueueName = null;
  configurer.UseSqlServerInLeaseMode(transportOptions);
}

But I erred on the side of (eventually) trying to limit the number of overloads.

The other option I played with is the idea of having an option builder... but found it felt like a lot of ceremony. Eg.

config.Transport(
  c => {
    SqlServerTransportOptionsBuilder builder = new SqlServerTransportOptionsBuilder();
    SqlServerTransportOptions options = builder
      .WithConnection(someDatabaseConnectionString)
      .AsOneWayClient()
      .SkipTableCreation()
      .Build();

    c.UseSqlServer(options)
  }
);

Rebus is MIT-licensed. The code submitted in this pull request needs to carry the MIT license too. By leaving this text in, I hereby acknowledge that the code submitted in the pull request has the MIT license and can be merged with the Rebus codebase.

mookid8000 commented 4 years ago

hmmm... other transports (e.g. RabbitMQ and Azure Service Bus) have their configuration extension on StandardConfigurer<ITransport> return an options builder, which can then have further calls made on it.

This way, the required things are always passed in in the Use<whatever> part, and then all the optional stuff can be customized by making additional calls.

With Azure Service Bus, a possible configuration could look like this:

Configure.With(_activator)
    .Transport(t => {
        t.UseAzureServiceBus(ConnectionString, QueueName)
            .AutomaticallyRenewPeekLock();
    })
    .Start();

Maybe one could imagine something like this for SQL Server:

Configure.With(...)
    .Transport(t => {
        t.UseSqlServer(connectionString, "my-queue")
            .EnsureTablesAreCreated(enabled: false);
    })
    .Start();

?

MrMDavidson commented 4 years ago

Okay, I'll switch to this style and update the PR. Just a note that the only required parameter for SQL is "a way to connect to the DB" (Eg. Connection string, IDBConnection factory method, or IDBConnectionProvider. Even the queue name is optional because it could be a one way transport (not sure if that's the same with other transports?).

MrMDavidson commented 4 years ago

I ended up with a partial approach. You still pass in the transport options ... but it gets returned and there's a fluent interface for setting everything. So it ends up being like;

Configure.With(...)
    .Transport(t => {
        t.UseSqlServer(new SqlServerTransportOptions(connectionString))
            .ReadFrom("my-queue")
            .EnsureTablesAreCreated(enabled: false);
    })
    .Start();

Reason being; there's 3 ways to create the DB connection. Which means we end up with 6 different use overloads. (3 DB connection variants for lease vs normal SQL transport and if you have one way clients as a first class citizen, it's even more).

How do you feel about a hybrid approach? You still get the nicer fluent API.. but we also still manage to slim down the API entry point surface for the transport.

mookid8000 commented 4 years ago

Sorry for not being entirely clear on this, but I actually think it's pretty important that the configuration pattern is the same across all transports.... the pattern looks like this:

.Transport(t => t.Use<name-of-transport>(<required-parameters>, queueName))

.Transport(t => t.Use<name-of-transport>AsOneWayClient(<required-parameters>))

which with SQL Server would translate into this:

.Transport(t => t.UseSqlServer(connectionString, queueName))

.Transport(t => t.UseSqlServerAsOneWayClient(connectionString))

UseSqlServer could then return an options builder relevant to a Rebus instance that can receive messages, while UseSqlServerAsOneWayClient would return a builder for options relevant to a client.

This might not be completely ideal.... the design follows from a time where there were no such thing as a "one-way client"..... but I've later realized that a better configuration pattern would look like this:

Configure.Server(queueName)
    .Transport(t => t.UseSqlServer(connectionString))
    .Start();

Configure.Client()
    .Transport(t => t.UseSqlServer(connectionString))
    .Start();

i.e. where the queue name is "global" to the configuration.

All IoC container packages could then hook themselves up to Rebus by providing an extension method appropriate to that container, so that e.g. Microsoft Extensions DI would be something like

// to configure it
serviceProvider.AddRebus(
    configure => configure.Server(queueName)
        .Transport(t => t.UseSqlServer(connectionString))
);

// to start it
serviceProvider.UseRebus();

or with Autofac

builder.RegisterRebus(
    configure => configure.Server(queueName)
        .Transport(t => t.UseSqlServer(connectionString))
);

// building the container starts the bus
using(var container = builder.Build())
{
     // 
}

but I'm afraid that's a separate (and much bigger) discussion.

so...

back to the MSSQL transport API.... can I maybe convince you that following the same pattern as the other transports would be a good idea? 😁

MrMDavidson commented 4 years ago

Sorry for not being entirely clear on this, but I actually think it's pretty important that the configuration pattern is the same across all transports.... the pattern looks like this:

.Transport(t => t.Use<name-of-transport>(<required-parameters>, queueName))

.Transport(t => t.Use<name-of-transport>AsOneWayClient(<required-parameters>))

which with SQL Server would translate into this:

.Transport(t => t.UseSqlServer(connectionString, queueName))

.Transport(t => t.UseSqlServerAsOneWayClient(connectionString))

UseSqlServer could then return an options builder relevant to a Rebus instance that can receive messages, while UseSqlServerAsOneWayClient would return a builder for options relevant to a client.

Can you confirm the part that's missing? The new API entry points now are;

.Transport(t => t.UseSqlServer(new SqlServerTransportOptions(...))

.Transport(t => t.UseSqlServerInLeaseMode(new SqlServerLeaseTransportOptions(...))

Both of these return the SqlServer[Lease]TransportOptions with a fluent interface. So actual configuration might be...

.Transport(t => t.UseSqlServerInLeaseMode(new SqlServerLeaseTransportOptions(...))
  .OptOutOfTableCreation()
  .ReadFromQueue("foo-message-queue")
  .EnableAutomaticLeaseRenewal(TimeSpan.FromMinutes(2));

Happy to make the two *AsOneWayClient entry points and then modify UseSqlServer and UseSqlServerInLeaseMode to take the queue name and "promote" as a required thing. This would then end up with...

.Transport(t => t.UseSqlServer(new SqlServerTransportOptions(...), inputQueueName))
.Transport(t => t.UseSqlServerAsOneWayClient(new SqlServerTransportOptions(...))

.Transport(t => t.UseSqlServerInLeaseMode(new SqlServerLeaseTransportOptions(...), inputQueueName))
.Transport(t => t.UseSqlServerInLeaseModeAsOneWayClient(new SqlServerLeaseTransportOptions(...))

But removing the SqlServerTransportOptions as the parameter becomes messy because you can create the transport with either;

  1. A connection string
  2. A Func<Task<IDbConnection>>
  3. A IDbConnectionProvider

Which would then mean instead of having 4 potential entry points ([Lease | Transactional] x [One Way | Two Way]) you end up with 12 entry points ([Connection String | Func | IDbConnectionProvider] x [Lease | Transactional] x [One Way | Two Way]). And then if another option is added, it has to be added to all those variants. 

.Transport(t => t.UseSqlServer(connectionString, inputQueueName))
.Transport(t => t.UseSqlServer(dbConnectionFactory, inputQueueName))
.Transport(t => t.UseSqlServer(dbconnectionProvider, inputQueueName))
.Transport(t => t.UseSqlServerAsOneWayClient(connectionString))
.Transport(t => t.UseSqlServerAsOneWayClient(dbConnectionFactory))
.Transport(t => t.UseSqlServerAsOneWayClient(dbconnectionProvider))

.Transport(t => t.UseSqlServerInLeaseMode(connectionString, inputQueueName))
.Transport(t => t.UseSqlServerInLeaseMode(dbConnectionFactory, inputQueueName))
.Transport(t => t.UseSqlServerInLeaseMode(dbconnectionProvider, inputQueueName))
.Transport(t => t.UseSqlServerInLeaseModeAsOneWayClient(connectionString))
.Transport(t => t.UseSqlServerInLeaseModeAsOneWayClient(dbConnectionFactory))
.Transport(t => t.UseSqlServerInLeaseModeAsOneWayClient(dbconnectionProvider))
mookid8000 commented 4 years ago

Yeah, well.... 🙄 the

.Transport(t => t.UseSqlServer(connectionString, inputQueueName))
.Transport(t => t.UseSqlServer(dbConnectionFactory, inputQueueName))
.Transport(t => t.UseSqlServer(dbconnectionProvider, inputQueueName))
.Transport(t => t.UseSqlServerAsOneWayClient(connectionString))
.Transport(t => t.UseSqlServerAsOneWayClient(dbConnectionFactory))
.Transport(t => t.UseSqlServerAsOneWayClient(dbconnectionProvider))

.Transport(t => t.UseSqlServerInLeaseMode(connectionString, inputQueueName))
.Transport(t => t.UseSqlServerInLeaseMode(dbConnectionFactory, inputQueueName))
.Transport(t => t.UseSqlServerInLeaseMode(dbconnectionProvider, inputQueueName))
.Transport(t => t.UseSqlServerInLeaseModeAsOneWayClient(connectionString))
.Transport(t => t.UseSqlServerInLeaseModeAsOneWayClient(dbConnectionFactory))
.Transport(t => t.UseSqlServerInLeaseModeAsOneWayClient(dbconnectionProvider))

API is present right now, if I remember correctly.... but I agree with you that the number of overloads is pretty overwhelming....

mookid8000 commented 4 years ago

hey @MrMDavidson , you have made some great contributions to Rebus over the years, and it's hard for me to keep up.... would you maybe be interested in becoming an official maintainer of Rebus.SqlServer? Hit me up on mogens@rebus.fm if you're interested 😄

MrMDavidson commented 4 years ago

Yeah, you're correct on that front, they are there now but marker as obsolete (in this PR) to allow people a reasonable upgrade path. And then at some point (hopefully not too long!) we'd drop them. That said, this is a major version jump so breaking API changes are okay.

From: Mogens Heller Grabe notifications@github.com Sent: Friday, August 30, 2019 3:08:16 AM To: rebus-org/Rebus.SqlServer Rebus.SqlServer@noreply.github.com Cc: Michael Davidson me@michaeldavidson.me; Author author@noreply.github.com Subject: Re: [rebus-org/Rebus.SqlServer] Introduce Transport Options (#53)

Yeah, well.... 🙄 the

.Transport(t => t.UseSqlServer(connectionString, inputQueueName))

.Transport(t => t.UseSqlServer(dbConnectionFactory, inputQueueName))

.Transport(t => t.UseSqlServer(dbconnectionProvider, inputQueueName))

.Transport(t => t.UseSqlServerAsOneWayClient(connectionString))

.Transport(t => t.UseSqlServerAsOneWayClient(dbConnectionFactory))

.Transport(t => t.UseSqlServerAsOneWayClient(dbconnectionProvider))

.Transport(t => t.UseSqlServerInLeaseMode(connectionString, inputQueueName))

.Transport(t => t.UseSqlServerInLeaseMode(dbConnectionFactory, inputQueueName))

.Transport(t => t.UseSqlServerInLeaseMode(dbconnectionProvider, inputQueueName))

.Transport(t => t.UseSqlServerInLeaseModeAsOneWayClient(connectionString))

.Transport(t => t.UseSqlServerInLeaseModeAsOneWayClient(dbConnectionFactory))

.Transport(t => t.UseSqlServerInLeaseModeAsOneWayClient(dbconnectionProvider))

API is present right now, if I remember correctly.... but I agree with you that the number of overloads is pretty overwhelming....

MrMDavidson commented 4 years ago

I opted for the four methods;

.Transport(t => t.UseSqlServer(new SqlServerTransportOptions(...), inputQueueName))
.Transport(t => t.UseSqlServerAsOneWayClient(new SqlServerTransportOptions(...))

.Transport(t => t.UseSqlServerInLeaseMode(new SqlServerLeaseTransportOptions(...), inputQueueName))
.Transport(t => t.UseSqlServerInLeaseModeAsOneWayClient(new SqlServerLeaseTransportOptions(...))

There's also these further methods (obsolete, for future removal);

public static void UseSqlServerInLeaseModeAsOneWayClient(this StandardConfigurer<ITransport> configurer, string connectionString, TimeSpan? leaseInterval = null, TimeSpan? leaseTolerance = null, bool automaticallyRenewLeases = false, TimeSpan? leaseAutoRenewInterval = null, Func<string> leasedByFactory = null, bool enlistInAmbientTransaction = false, bool ensureTablesAreCreated = true);
public static void UseSqlServerInLeaseModeAsOneWayClient(this StandardConfigurer<ITransport> configurer, Func<Task<IDbConnection>> connectionFactory, TimeSpan? leaseInterval = null, TimeSpan? leaseTolerance = null, bool automaticallyRenewLeases = false, TimeSpan? leaseAutoRenewInterval = null, Func<string> leasedByFactory = null);
public static void UseSqlServerInLeaseMode(this StandardConfigurer<ITransport> configurer, string connectionString, string inputQueueName, TimeSpan? leaseInterval = null, TimeSpan? leaseTolerance = null, bool automaticallyRenewLeases = false, TimeSpan? leaseAutoRenewInterval = null, Func<string> leasedByFactory = null, bool enlistInAmbientTransaction = false, bool ensureTablesAreCreated = true);
public static void UseSqlServerInLeaseMode(this StandardConfigurer<ITransport> configurer, Func<Task<IDbConnection>> connectionFactory, string inputQueueName, TimeSpan? leaseInterval = null, TimeSpan? leaseTolerance = null, bool automaticallyRenewLeases = false, TimeSpan? leaseAutoRenewInterval = null, Func<string> leasedByFactory = null, bool ensureTablesAreCreated = true);
public static void UseSqlServerAsOneWayClient(this StandardConfigurer<ITransport> configurer, Func<Task<IDbConnection>> connectionFactory);
public static void UseSqlServerAsOneWayClient(this StandardConfigurer<ITransport> configurer, string connectionString, bool enlistInAmbientTransaction = false);
public static void UseSqlServer(this StandardConfigurer<ITransport> configurer, Func<Task<IDbConnection>> connectionFactory, string inputQueueName, bool ensureTablesAreCreated = true);
public static void UseSqlServer(this StandardConfigurer<ITransport> configurer, string connectionString, string inputQueueName, bool enlistInAmbientTransaction = false, bool ensureTablesAreCreated = true);

You good if we merge this?

mookid8000 commented 4 years ago

You good if we merge this?

Sure! 🤠