Skip to content

Conversation

@no-reply
Copy link
Member

Repositories that don't support :atomic_write shouldn't try to use a transaction for #delete_insert.

Repositories answering true to :atomic_write promise to deliver an atomic #apply_changeset.

@no-reply
Copy link
Member Author

Does this address the remaining issues on #190 ?

(I can't test the other repositories at the moment, but will try to circle back soon).

Repositories that don't support :atomic_write shouldn't try to use a
transaction for #delete_insert.

Repositories answering `true` to `:atomic_write` promise to deliver an
atomic `#apply_changeset`.
@no-reply no-reply force-pushed the feature/no-transactional-delete-insert branch from a87c621 to 80ff975 Compare February 17, 2016 01:33
@no-reply
Copy link
Member Author

I was able to steal a moment to test SPARQL::Client::Repository, this fixes it, except for some largely unrelated issues. See: ruby-rdf/sparql-client@dda25c7

no-reply pushed a commit that referenced this pull request Feb 17, 2016
…-insert

Only use tx on #delete_insert for :atomic_write
@no-reply no-reply merged commit a4f85c8 into develop Feb 17, 2016
@no-reply no-reply deleted the feature/no-transactional-delete-insert branch February 17, 2016 02:28
@no-reply no-reply mentioned this pull request Feb 17, 2016
@gkellogg
Copy link
Member

Looks like that should do it, but I remain a bit unclear on the underlying theory, and what a repository implementation should do. Why starting a transaction in #delete_insert is okay in one case (and doesn't lead to recursion), but not in another isn't clear to me.

Is there some guidance on how rdf-mongo, or rdf-do should implement this? Certainly, in the case of rdf-do, there is an underlying transaction model that could be use; how would a repository factor this in?

@no-reply
Copy link
Member Author

The base implementation is:

  • Mutable#delete_insert does #delete and #insert (non-atomic).
  • Mutable#apply_changeset calls #delete_insert (non-atomic)
  • Transaction#execute calls #apply_changeset

If a Repository supports :atomic_write it must provide both of these methods with atomic updates. #apply_changeset is the key method (see Transaction's YARD annotations); any repository that implements :atomic_write needs to have custom changeset application logic. #delete_insert must be transactional.

So, if :atomic_write is supported:

  • Repository#delete_insert runs a transaction
    • It falls back on Mutable#delete_insert (and avoids a transaction) for non-atomic repositories.
  • Repository#apply_changeset gets a custom implementation, and doesn't call #delete_insert.

That leaves Transaction#execute. Transaction subclasses may provide their own implementation of this, avoiding #apply_changeset altogether. For example, they might send an upstream request to commit (or do @data = tx_data as in Implementation). In this case, it is permissible to implement #apply_changeset in terms of transactions, as well. This would be up to the repository.

I'm open to suggestions on simplifying this. For the moment, the simple path to changeset-driven transactions is a one-step, atomic #apply_changeset.

@gkellogg
Copy link
Member

No, I think that's fine. We should eventually update rdf-do to implement this transaction/atomic-write pattern to serve as an example for other repos. I'll see if we can do something similar with Mongo, but I doubt it.

@no-reply
Copy link
Member Author

I'll take a hack at some more thorough transaction docs targeted at Repository implementers later in the week.

I'm trying to use my limited time on this carefully. My todo, in order, is:

@gkellogg
Copy link
Member

Sure, updating rdf-do is not urgent, other than to make it basically work (which it should, now).

Regarding the blog, for this particular case, we should probably use the GitHub release notes on RDF.rb, once the release is tagged.

Even though it would be nice to get things out, we're not on a clock, and we're not being paid to do the work, so work on whatever schedule you're comfortable with.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants