Remove transaction support? #205

Closed
tfausak opened this Issue Aug 7, 2014 · 8 comments

Comments

Projects
None yet
3 participants
@tfausak
Collaborator

tfausak commented Aug 7, 2014

Transactions have been available since 0.2.0 (#20) and configurable since 1.2.0 (#155). We originally added them because we thought interactions would be used for managing complex business logic, which probably involved multiple models. In practice, that isn't always the case.

Instead of providing a thin wrapper around ActiveRecord transactions, it might be better to provide nothing. If you wanted to use transactions in an interaction, you'd have to do it yourself. For example:

# As it is currently:
class AutomaticTransactionInteraction < ActiveInteraction::Base
  def execute
    # ...
  end
end

# As it might be:
class ManualTransactionInteraction < ActiveInteraction::Base
  def execute
    ActiveRecord::Base.transaction do
      # ...
    end
  end
end

@tfausak tfausak added this to the v2.0.0 milestone Aug 7, 2014

@tfausak tfausak added the question label Aug 7, 2014

@AaronLasseigne

This comment has been minimized.

Show comment
Hide comment
@AaronLasseigne

AaronLasseigne Aug 7, 2014

Owner

When we expected interactions to manage data handling among multiple models the transaction made sense. However, after observing its use over time it has proven to be useful for much more than that. I've seen lots of situations that do not need transaction support. I'm increasingly leaning toward removing transaction support all together. Of course, this would have to be a 2.0 change.

Owner

AaronLasseigne commented Aug 7, 2014

When we expected interactions to manage data handling among multiple models the transaction made sense. However, after observing its use over time it has proven to be useful for much more than that. I've seen lots of situations that do not need transaction support. I'm increasingly leaning toward removing transaction support all together. Of course, this would have to be a 2.0 change.

@clifton

This comment has been minimized.

Show comment
Hide comment
@clifton

clifton Aug 7, 2014

Perhaps having a delegate method on AI::B to AR::B.transaction is a nice compromise. It could later be extended (or have configuration options) to allow pipelining commands to redis or whatever the end user is using as their primary data store.

clifton commented Aug 7, 2014

Perhaps having a delegate method on AI::B to AR::B.transaction is a nice compromise. It could later be extended (or have configuration options) to allow pipelining commands to redis or whatever the end user is using as their primary data store.

@clifton

This comment has been minimized.

Show comment
Hide comment
@clifton

clifton Aug 7, 2014

And it's less typing 🍺

clifton commented Aug 7, 2014

And it's less typing 🍺

@clifton

This comment has been minimized.

Show comment
Hide comment
@clifton

clifton Aug 7, 2014

Or maybe it would be preferable to allow customization for layered transactions so they could be tagged and allow more uniform logging over multiple DBs (redis, SQL). Then again, that sounds like it should be its own library orgsync/atomic anyone?

clifton commented Aug 7, 2014

Or maybe it would be preferable to allow customization for layered transactions so they could be tagged and allow more uniform logging over multiple DBs (redis, SQL). Then again, that sounds like it should be its own library orgsync/atomic anyone?

@AaronLasseigne

This comment has been minimized.

Show comment
Hide comment
@AaronLasseigne

AaronLasseigne Aug 7, 2014

Owner

Agreed. Behavior like that is probably it's own library. It sounds useful though.

Owner

AaronLasseigne commented Aug 7, 2014

Agreed. Behavior like that is probably it's own library. It sounds useful though.

@clifton

This comment has been minimized.

Show comment
Hide comment
@clifton

clifton Aug 7, 2014

For sure. Especially in cases where you have multiple layers of transactions and need to support graceful failure and enough logging to understand what happened if things go wrong.

clifton commented Aug 7, 2014

For sure. Especially in cases where you have multiple layers of transactions and need to support graceful failure and enough logging to understand what happened if things go wrong.

@tfausak

This comment has been minimized.

Show comment
Hide comment
@tfausak

tfausak Aug 14, 2014

Collaborator

I'm in favor of removing support for transactions and, by extension, not wrapping interactions in transactions by default. @AaronLasseigne hit the nail on the head with the observation that interactions don't always involve multiple classes. As far as maintaining the #transaction method that @clifton suggested, I don't think that's useful enough to warrant.

Collaborator

tfausak commented Aug 14, 2014

I'm in favor of removing support for transactions and, by extension, not wrapping interactions in transactions by default. @AaronLasseigne hit the nail on the head with the observation that interactions don't always involve multiple classes. As far as maintaining the #transaction method that @clifton suggested, I don't think that's useful enough to warrant.

@AaronLasseigne

This comment has been minimized.

Show comment
Hide comment
@AaronLasseigne

AaronLasseigne Aug 14, 2014

Owner

Well, I believe the ayes have it.

Owner

AaronLasseigne commented Aug 14, 2014

Well, I believe the ayes have it.

tfausak added a commit that referenced this issue Aug 15, 2014

@tfausak tfausak self-assigned this Aug 15, 2014

tfausak added a commit that referenced this issue Aug 15, 2014

Merge branch 'v2.0.0' into gh-205
Conflicts:
	lib/active_interaction/concerns/runnable.rb

tfausak added a commit that referenced this issue Aug 15, 2014

@tfausak tfausak closed this Aug 15, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment