Suggest Method Names for DBIx::Connector

Thanks to feedback from Tim Bunce and Peter Rabbitson in a DBIx::Class bug report, I've been reworking DBIx::Connector's block-handling methods. Tim's objection is that the the feature of do() and txn_do() that executes the code reference a second time in the event of a connection failure can be dangerous. That is, it can lead to action-at-a-distance bugs that are hard to find and fix. Tim suggested renaming the methods do_with_retry() and txn_do_with_retry() in order to make explicit what's going on, and to have non-retry versions of the methods.

I've made this change in the repository. But I wasn't happy with the method names; even though they're unambiguous, they are also overly long and not very friendly. I want people to use the retrying methods, but felt that the long names make the non-retrying preferable to users. While I was at it, I also wanted to get rid of do(), since it quickly became clear that it could cause some confusion with the DBI's do() method.

I've been thesaurus spelunking for the last few days, and have come up with a few options, but would love to hear other suggestions. I like using run instead of do to avoid confusion with the DBI, but otherwise I'm not really happy with what I've come up with. There are basically five different methods (using Tim's suggestions for the moment):

run( sub {} )
Just run a block of code.
txn_run( sub {} )
Run a block of code inside a transaction.
run_with_retry( sub {} )
Run a block of code without pinging the database, and re-run the code if it throws an exception and the database turned out to be disconnected.
txn_run_with_rerun( sub {} )
Like run_with_retry(), but run the block inside a transaction.
svp_run( sub {} )
Run a block of code inside a savepoint (no retry for savepoints).

Here are some of the names I've come up with so far:

Run block Run in txn Run in savepoint Run with retry Run in txn with retry Retry Mnemonic
run txn_run svp_run runup txn_runup Run assuming the db is up, retry if not.
run txn_run svp_run run_up txn_run_up Same as above.
run txn_run svp_run rerun txn_rerun Run assuming the db is up, rerun if not.
run txn_run svp_run run::retry txn_run::retry :: means “with”

That last one is a cute hack suggested by Rob Kinyon on IRC. As you can see, I'm pretty consistent with the non-retrying method names; it's the methods that retry that I'm not satisfied with. A approach I've avoided is to use an adverb for the non-retry methods, mainly because there is no retry possible for the savepoint methods, so it seemed silly to have svp_run_safely() to complement do_safely() and txn_do_safely().

Brilliant suggestions warmly appreciated.

Backtalk

Tim Bunce wrote:

Or don't have separate methods for the with and without retry variants. Perhaps something like this:

$obj->run(sub { ... });

$obj->run({ retry => 1 }, sub { ... });

having some way to pass options could be be useful anyway to offer more control over when a retry should or shouldn't be done.

notbenh wrote:

I get to have a title?

it seems that your combining two distinct ideas: rerun on error & do something. Seems that it might be better to just break those ideas?

$obj->re->run(sub{}); $obj->re->tnx_do(sub{});

Theory wrote:

Re: I get to have a title?

Ben,

No, actually the re-run only happens if the block dies and the method finds that the handle isn't disconnected. If the block dies and the handle is connected, it just lets the die pass through.

—Theory

Theory wrote:

Tim,

I think the reason not to do that is that it's not just a retry. It's also the case that it doesn't ping before executing the code! To me, that's an extra effect not adequately expressed in a parameter, but makes more sense as a different method. Basically, it's completely changing the context in which the block is executed. A separate method name is a better expression of that, IMHO.

—Theory

Brig wrote:

In the "w/ retry" case, you "fuss" with the code if things aren't quite right. So how about: "run_fuss" for "run_with_retry" and "txn_run_fuss" for "txn_run_with_rerun"? Or, since you are taking extra action if things misfire, i.e. you are certifying or being certain, how about "run_cert" and "txn_run_cert"?

Brig wrote:

of course, maybe 'cert' triggers 'certification' or 'certificate' associations. so maybe 'sure' would be a little softer but still illustrative: 'run_sure' and 'txn_run_sure'

Mark Lawrence wrote:

For the case where no transaction or savepoints are being used, I'm actually wondering if the whole "run code block with retry on disconnected handle" is approaching the problem at the right level. Why would a user want an entire block to be rerun instead of just re-executing the failing DBI method?

Ie - did you consider sub-classing DBI::db methods instead. Very, very pseudo code to follow:

sub execute {
    eval { SUPER::execute( @_ ) };
    if ( $@ and ping_failed ) {
        if ( re_connect() ) {
            SUPER::execute( @_ );
        }
        else {
            die $@;
        }
    }
}

Mark Lawrence wrote:

I just had another idea while looking at this bug report from github. How about extending the try/catch idea I suggested to be try/retry/catch:

Do the transaction:

$conn->txn( sub{
    my $dbh = shift;
    $dbh->do('INSERT INTO table1 VALUES (1)');
});

Do the transaction, and catch any error:

$conn->txn( sub{
    my $dbh = shift;
    $dbh->do('INSERT INTO table1 VALUES (1)');
},
catch {
    # Handle the error
});

Do the transaction, retry on disconnect, catch

$conn->txn( sub{
    my $dbh = shift;
    $dbh->do('INSERT INTO table1 VALUES (1)');
},
retry_on_disconnect,
catch {
    # Handle the error
});

Thoughts?

Theory wrote:

Mark,

You don't want to re-execute a single DBI call because any work that comes before it in a transaction will be thrown out. And I certainly don't want to get into the business of subclassing/second-guessing the DBI!

I like the idea of adding catch support, though; Once I have the main API worked out (and I'm pretty close now), I'll have a think about adding the catch feature.

Mark Lawrence wrote:

run() outside of transactions

Mark wrote:

For the case where no transaction or savepoints are being used...

Then Theory wrote:

You don't want to re-execute a single DBI call because any work that comes before it in a transaction will be thrown out...

I don't think you quite got my point. I am questioning the need for the "run()" method. Outside of any transaction I have no need for my code to be re-run, if the DBI handle could redo the DBI call on its own. i.e. the only way I would see to use run() is like this:

$conn->run( sub {
    $conn->dbh->prepare(...);
} );
$conn->run( sub {
    $conn->dbh->execute(...);
} );

Instead of doing the prepare, and execute, and whatever all over again in the event of disconnection:

$conn->run( sub {
    $conn->dbh->prepare(...); # why do this twice?
    $conn->dbh->execute(...);
} );

Mark wrote:

Run with retry

Sorry, I was actually talking about the "Run with retry" method.

Theory wrote:

Re: run() outside of transactions

Mark,

You're right, I missed that you were talking about the non-transactional function. However, the run() function does not re-run; the run::retry() method does.

However, a lot of this is moot now. I've finally nailed things down and will do a release later today. I'll blog about the changes (substantial, I'm afraid) here as well.

—Theory

Ron Savage wrote:

Hi

Firstly, why not drop the suffix '_run' to leave txn and svp?

Secondly, why not add the retry as a prefix: retry_txn?

Theory wrote:

Ron,

See my followup for what I actually ended up doing.

Thanks,

Theory