U4-7887 - Make PetaPoco CommandTimeout configurable

Created by Jeavon Leopold 03 Feb 2016, 11:16:44 Updated by Shannon Deminick 04 Feb 2016, 16:30:24

Relates to: U4-7869

Some queries can take more than the default 30 second command timeout to execute especially when running on Azure with S0 type databases. Therefore it should be possible to configure the command timeout in PetaPoco.

There is a SQL Connection timeout in the connection string, "Connection Timeout" but this isn't the same as Command timeout however I think it would work pretty well as a way of allowing Command timeout to be configured without having to add a new AppSetting.

This is what I have tested successfully adding the second "else if":

void DoPreExecute(IDbCommand cmd) { // Setup command timeout if (OneTimeCommandTimeout != 0) else if (CommandTimeout!=0) // No OneTimeCommandTimeout or CommandTimeout so lets use the ConnectionTimeout value from web.config if there is one and it's greater than the default 30 else if (cmd.Connection.ConnectionTimeout > 30) // Call hook OnExecutingCommand(cmd);

		// Save it
		_lastSql = cmd.CommandText;
		_lastArgs = (from IDataParameter parameter in cmd.Parameters select parameter.Value).ToArray();
	}

Example connection string: Server=tcp:mything.database.windows.net,1433;Database=MyThing;User ID=user@mything;Password=superduperpassword;Encrypt=True;TrustServerCertificate=False;Connection Timeout=300

Comments

Stephan 04 Feb 2016, 11:36:25

Wondering if, in order to keep PetaPoco.Database code untouched (to facilitate upgrades) we should instead set CommandTimeout in UmbracoDatabase constructor?


Jeavon Leopold 04 Feb 2016, 11:42:24

Sadly, there are no PetaPoco upgrades. PetaPoco.Database code is already drastically different to the original with fixed bits copied from NPoco and other custom code but with the switch to NPoco for v8 maybe worth thinking about what happens with that or cross that bridge later....?


Stephan 04 Feb 2016, 12:07:41

As you say most changes come from NPoco and I'd rather not add things that are not in NPoco (though we might have done it already...). I ''think'' just setting CommandTimeout in UmbracoDatabase constructor would achieve the same result but may be wrong?


Shannon Deminick 04 Feb 2016, 12:26:23

Let's try not to hack the petapoco core more than we need to. Yes there are already changes but we'll keep those to a minimum. We will be switching to NPoco for v8... though some devs have picked up PetaPoco again so we'll see how they go (my guess is not very far).

I don't mind how we achieve this, if it can be as simple as a ctor argument, sounds good to me


Jeavon Leopold 04 Feb 2016, 12:30:16

Yes, you can use the UmbracoDatabase constructor but will need to get the value from somewhere or parse it out of the connection string if using the Connection Timeout value. Hard coded values work: public UmbracoDatabase(IDbConnection connection, ILogger logger) : base(connection)

    public UmbracoDatabase(string connectionString, string providerName, ILogger logger)
        : base(connectionString, providerName)
    {
        _logger = logger;
        EnableSqlTrace = false;
        CommandTimeout = 300;
    }

    public UmbracoDatabase(string connectionString, DbProviderFactory provider, ILogger logger)
        : base(connectionString, provider)
    {
        _logger = logger;
        EnableSqlTrace = false;
        CommandTimeout = 300;
    }

    public UmbracoDatabase(string connectionStringName, ILogger logger)
        : base(connectionStringName)
    {
        _logger = logger;
        EnableSqlTrace = false;
        CommandTimeout = 300;
    }


Stephan 04 Feb 2016, 12:42:31

add something along if (CommandTimeout == 0) CommandTimeout = connection.ConnectionTimeout; in UmbracoDatabase's override of OnConnectionOpened?


Stephan 04 Feb 2016, 12:43:48

or on OnExecutingCommand


Stephan 04 Feb 2016, 12:45:14

ie just add public override void OnExecutingCommand(IDbCommand cmd) { if (cmd.CommandTimeout == 0 && cmd.Connection.ConnectionTimeout > 30) cmd.CommandTimeout = cmd.Connection.ConnectionTimeout; base.OnExecutingCommand(cmd); } to UmbracoDatabase?


Jeavon Leopold 04 Feb 2016, 12:54:43

That looks good but I think it would be like this? public override void OnExecutingCommand(IDbCommand cmd) { if (OneTimeCommandTimeout == 0 && CommandTimeout == 0 && cmd.Connection.ConnectionTimeout > 30) cmd.CommandTimeout = cmd.Connection.ConnectionTimeout; base.OnExecutingCommand(cmd); }


Jeavon Leopold 04 Feb 2016, 12:59:58

By default cmd.CommandTimeout will be 30


Stephan 04 Feb 2016, 13:25:37

so we'd use cmd.Connection.ConnectionTimeout ''only'' if it hasn't been set previously via OneTimeCommandTimeout or CommandTimeout? Makes sense. OK, will do.


Jeavon Leopold 04 Feb 2016, 13:51:32

yes and by default cmd.CommandTimeout will be 30 but by default cmd.Connection.ConnectionTimeout is 15 hence the need for the > 30


Stephan 04 Feb 2016, 13:51:48

PR https://github.com/umbraco/Umbraco-CMS/pull/1097


Priority: Normal

Type: Feature (request)

State: Fixed

Assignee:

Difficulty: Normal

Category:

Backwards Compatible: True

Fix Submitted:

Affected versions:

Due in version: 7.4.0

Sprint: Sprint 8

Story Points:

Cycle: