There seems to be no real isolation between queues. This is problematic for a variety of reasons. From a performance perspective everything is serialized through your Que.Server genserver, which won’t scale as you have more queues or as particular queues become very busy.
It’s also an issue from a fault tolerance perspective. Any issue with one queue will nuke every other queue.
Each job is executed in its own process which is good, but by putting them under all the same supervisor with the same settings all you need is 5 job failures in 3 seconds to take down all the other jobs. You can of course just adjust the supervisor settings but even still, different supervisors for different queues would be better.
The root issue is that you do 99% of the stuff with a single genserver. There also appear to be race conditions. For example, two nodes starting at the same time will both query mnesia for incomplete jobs. You do that in a transaction, so one will run after another, but since you don’t update the state in that transaction they’re both still gonna get the same list of incomplete jobs and both try to run them.
I apologize that all of this is so negative. The issue is that while Erlang and Elixir are indeed good fits for the items on that list you have, the reason that they’re good is because Erlang and Elixir offer excellent primitives with which to solve the problem. Those primitives still need to be used correctly, and doing so can actually be pretty hard because the problems themselves are pretty hard.
Only comment I’d have here is that there is a Ruby library called Que that uses Postgres advisory locks for some speed perks and has gained in popularity a good bit.
Just to avoid naming conflicts it might be a good idea to change the name to something more distinctive? I’d heard that there was potentially going to be an Elixir port of that library and if that happens you’re going to get a lot more confusion. If it was a library from any other language I wouldn’t be that concerned.
It is a queue. I’m placing jobs at the end and pulling them from the front. I guess the push and pop method names here are misleading. I should change them to something like in and out.
This is an interesting issue. I don’t know much about OTP to understand how to approach this. Should I create multiple Supervisors under one application, one supervisor supervising multiple supervisors, each for one worker or one supervisor with multiple GenServers each for one Worker? Either way, how can I automatically start the supervisors / genservers for each defined worker? Any tips on doing this the right way?
Not at all. I really appreciate you taking the time out to go through the code and give feedback. My goal is to improve my Elixir and OTP skills.
This is true, my apologies. Unfortunately however its implementation is such that adding N jobs distinctly produces N^2 work. If you do:
for image <- images do
you’re going to do N^2 work, when only linear is necessary if using something like :queue, or following its internal implementaiton. Doing ++ to the end of a growing list is generally not what you want to do.
You provide an API like poolboy for example, where you have a child_spec function and then people place it in their supervision tree in their own application.
There’s a lot to say here, I’ll have to reply tomorrow.
Thank you for being patient with me and taking the time out to answer my questions. I went back, did some more research and started refactoring the application trying to implement your suggestions (the OTP-related ones for now):
I’d say a zipper is overkill here, as we are only ever appending at the end and reading from the front. You just need one of Okasaki’s purely functional queue algorithms, of which :queue and multiple Elixir wrappers on Hex such as e_queue are implementations.
True true, just of the aspect of doing it manually. And it is not like zippers are difficult, just keep a tuple of two elements, pull off the front of one, push to the front of the other, when the pull one is empty then swap them and reverse, it is quite efficient and simple.