concourse icon indicating copy to clipboard operation
concourse copied to clipboard

Calling stage() when already in a transaction should be a no-op

Open jtnelson opened this issue 9 years ago • 6 comments

When writing code that uses transactions, applications need to keep track of whether the client they are using is in a transaction so that they don't accidentally start a new one before committing.


public void methodA() {
    concourse.stage();
    try {
        // do some work
        concourse.commit();
    }
    catch(TransactionException e) {
        concourse.abort();
    }
}

public void methodB() {
    concourse.stage();
    try {
        // do some work
        methodA();
        concourse.commit();
    }
    catch(TransactionException e) {
        concourse.abort();
    }
}

In the example above, the work that methodA() is done when called from methodB is an extension of the work methodB is doing, so the fact that methodB is already in a transaction should already be considered by the client when methodA is called

I can't think of a reason why the behaviour of calling stage when already in a transaction is to create a new transaction. I think its best to simply make it a no-op and continue with the current transaction, if it exists. This should be a small change.

Each client should just to see if the transaction token is not null, and if that is the case just return immediately.

We could implement this on the server if we edited the thrift API to take a transaction token in the stage method.

jtnelson avatar Nov 11 '15 11:11 jtnelson

One thing to think about:

In the example above, methodA calls commit, which can prematurely commit the work that methodB is doing if the transactions are automatically merged. To handle this we would need to add logic in the server to keep track of how many times stage has been called and only truly commit when commit has been called enough times to match how often stage has been called.

I'm not crazy about doing that kind of work on the server, but am open to it. At a certain point it seems lik we're starting to implement nested transactions, which may or may not be a good thing. In general I think the semantics of nested transactions are complicated and I prefer to keep things simple, even if that means that the client should maintain its logic about whether it is in a transaction or not. But I can be convinced otherwise here.

jtnelson avatar Nov 11 '15 11:11 jtnelson

@jtnelson Could you please guide me on this?

iamutkarshtiwari avatar Jun 05 '16 21:06 iamutkarshtiwari

We can assigning a static counter inside stage() or commit() to check how many times stage has been called. This can be somewhat like this-

stage() called then-> counter ++ commit() called then-> counter --

if counter == 0:     either transactions haven't started yet or are completed. else:     nested transactions in process.     _specific logic needed here_

iamutkarshtiwari avatar Jun 05 '16 22:06 iamutkarshtiwari

@iamutkarshtiwari We can't use a static counter, because we'd need to keep track of transaction calls PER client. If we're going to have a counter, it'll need to either be something that embedded in the transaction token itself OR held within a collection that maps the transactiontoken to a counter

jtnelson avatar Jun 06 '16 11:06 jtnelson

@as577 take a look at this as first bug fix

jtnelson avatar Jun 15 '16 15:06 jtnelson

@jtnelson, just made a pull request for this issue

adisrini avatar Jun 22 '16 20:06 adisrini