Transactional Stack Implementation

[jaebradley]

No description provided.

[coveralls]

Coverage decreased (-0.8%) to 98.028% when pulling afe9479 on stack-with-transaction-block into 299e8d5 on master.

[visola]

Overall looks good.

You don't have a test for rolling back an outer transaction that had a committed inner transaction. From what I'm seeing in the code, it doesn't seem like that would work at all. But inner transactions need to be rolled back if an outer transaction gets rolled back. It's kind of a weird scenario but it is how things work.

[visola]

I don't like this type of code flow (using exception catching to make decisions). It would be better to just check if stack is empty before trying to peek, then rollback if you have actions to rollback and last throw the exception yourself.

[jaebradley]

Ooooo I like that much better!

[visola]

I don't understand the role of this stack. Why do you have to keep track of rolled back events?

[jaebradley]

Because I'm using the public push / pop methods in the implementation of the rollback method I need to check whether I'm in the middle of a rollback - else, I might not get to the same state pre-transaction.

[visola]

Why do you need to use the public methods? Can't you just call pop/push in the underlying stack directly? There's not much code there to reuse.

[visola]

I prefer to make explicit when you're handling BEGIN.

[jaebradley]

heh, I debated this for a while - definitely agree. Do you think the default should return or throw a RuntimeException? I have a mild preference for the latter (since it would be an unexpected case).

[visola]

I would throw a RuntimeException to protect agains future changes. Since the values are coming from an enum, the only possible way of it happening is when you (or someone) adds a new value to the enum and don't handle it appropriately.

[jaebradley]

@visola oh shoot, I forgot about the outer rollback affecting inner rollbacks (and I think the same for outer commits impacting inner commits?)...hmmm, I wonder what the best way of keeping track of the "right" rollback is? A unique identifier for a rollback? And then pop off the Stack of TransactionEvents until that identifier is found?

[visola]

I was thinking about this yesterday. One way to implement this would be to have an explicit Transaction object that holds the transaction events stack inside it and also a reference for a parent transaction.

But I'm not sure the one dimensional transaction events stack that you have wouldn't work. I think that you can try just adding a new event type called Commit and a counter that manages how deep in the transaction hierarchy you are. With those two I think you can handle all the cases.

[jaebradley]

@visola I'm not sure that the outer transaction commit case can even happen in my implementation since the begin method doesn't return an object to reference.

String first = "first";
String second = "second";
TransactionalStack<String> stack = new TransactionalStack<>();

// start first "outer" transaction
stack.begin();
stack.push(first);
// start second "nested" transaction
stack.begin();
stack.push(second);
// at this point, I can't call commit on the first transaction 
// I can only call it on the last transaction
stack.commit(); // commits nested transaction
stack.commit(); // commits outer transaction

[coveralls]

Coverage decreased (-0.2%) to 98.611% when pulling b1adafa on stack-with-transaction-block into 299e8d5 on master.

[visola]

I agree, but this scenario will still fail:

String first = "first";
String second = "second";
TransactionalStack<String> stack = new TransactionalStack<>();

// start first "outer" transaction
stack.begin();
stack.push(first);
// start second "nested" transaction
stack.begin();
stack.push(second);
stack.commit(); // commits nested transaction
stack.rollback(); // rolls back outer transaction
// State here should be as before the first begin

[jaebradley]

@visola ahhhh yes, that's correct.

Hmmm, I'm re-reading your comment.

I wonder if it's simpler to have a committedEvents Stack. If a rollback occurs and committedEvents is not empty, then rollback those changes first and then rollback any uncommitted changes?

I think this would work for the triple-nested transaction, where the second transaction gets rolled back after the third transaction gets committed.