Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix parallel preloading with 'fake' transaction behaviour #13

Closed
wants to merge 2 commits into from

Conversation

harmon25
Copy link

Hi @evadne!
Was playing around with this and tried running multiple preloads in parallel Repo.preload([:employee, :customer]) and it was failing with the following error:

function Etso.Adapter.in_transaction?/1 is undefined or private

Took a look and realized the adapter is missing the Ecto.Adapter.Transaction behaviour.
I presume this is because there is no transaction mechanism we can use for :ets?
Anyway - figured we could trick Ecto into thinking we are never in a transaction - and the parallel preload now works as expected.

Might be some side effects I have not run into, thoughts?

  • add transaction behaviour to adapter
  • add a test for parallel preloading

add transaction behaviour to adapter
add a test for parallel preloading
@evadne
Copy link
Owner

evadne commented Apr 6, 2021

Hello @harmon25

It is probably better to not fake support for transactions, and instead use:

Repo.preload(, in_parallel: false)

Just my opinion here of course.

ALTERNATIVELY… Something like this might work as well

def transaction(_, _, _) do
  {:error, :not_implemented}
end

def in_transaction?(_) do
  false
end

def rollback(_, _) do
  raise NotImplementedException
end

@harmon25
Copy link
Author

harmon25 commented Apr 7, 2021

I didnt love the idea of faking it either, but being able to preload in parallel is nice - and it should work in ets, right?

I like your alternate suggestion - so if someone tries to run Repo.transaction/3 / Repo.rollback/2 they get some good feedback - while still allowing parallel preloading.

I will update the PR and add another test or two!

@evadne
Copy link
Owner

evadne commented Apr 10, 2021

@harmon25 Indeed, since the requirement of parallel loading is “not in a transaction” (although I do not necessarily agree), our lack of support of transactions with ETS should not be an issue at all, IMO.

@evadne
Copy link
Owner

evadne commented May 5, 2021

@harmon25 Hey, how have you been? 🧠

raise exception when attempting rollback
@harmon25
Copy link
Author

harmon25 commented May 9, 2021

Hi @evadne, just a bit busy. Not been working on the project that relies on Etso - but I did update the PR to address your feedback! Hope you are well.

This did not work though, and broke all tests that are writing to the repo.

def transaction(_, _, _) do
  {:error, :not_implemented}
end

So kept that fake transaction behavior to avoid the compilation warning when it is not defined.

Attempting to rollback now raises an exception.

@evadne
Copy link
Owner

evadne commented Jun 4, 2021

OK, then in that case I think we need to do something else here

@evadne
Copy link
Owner

evadne commented Jul 5, 2021

I think we can’t cheat so have to do it properly, I found a paper from 10 years ago and I will try to look for something newer.

🤔

Paper is:

Erlang ETS Tables and Software Transactional Memory: How Transactions Make ETS Tables More Like Ordinary Actors — Patrik Nyblom, Ericsson AB [2011]

@evadne evadne closed this in 8fc610e May 20, 2022
@evadne
Copy link
Owner

evadne commented May 20, 2022

With Ecto 3.8 this has become much easier to do w/o needing to fake transactions. Have verified on develop that :in_parallel actually executes from various pids now

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.

2 participants