Looking for feedback on a little project I've made

Chat / Discussions
, , ,
1

Hello Elixir community!

I gave a few programming courses in a university a few months ago, and found myself with ~ 250 student projects to grade (from two assignments, introduction to Git & unit testing in Java).

So I jumped on the occasion to get my hands dirty with BEAM processes in the process! Thought it would be a pretty useful application of concurrency and fault-tolerance.

Since this is my first time using GenServer, Task and Supervisor/DynamicSupervisor, I would greatly appreciate any feedback you could make!

It does work very well, but I’m sure there is room for improvement, and I might even have made big mistakes.

You can find the project here: GitHub - sheerlox/auto_grader: An automatic assignments grading program written in Elixir.

The readme contains my initial thought process and (ignorant) first design draft.

I’ve also published ExDocs on GH pages which contain the actual up-to-date documentation on how this thing works.

Thanks in advance :pray:

1 Like
2

I took a peruse through the code, and my biggest suggestion would be to just use Task and Task.Supervisor instead of GenServer, which would probably clean the code up a bit (Task.Supervisor is a DynamicSupervisor!) You could also use PartitionSupervisor with it, but I don’t think invoking the supervisor will be your bottleneck if you only have 250-ish tasks.

All in all, nice job! We had some code at my job that looked pretty similar to this running an automated integration test suite against prod

2 Likes
3

Thanks for taking a look!

For some reason, I believed Task couldn’t monitor and receive messages from other Tasks, which doesn’t seem true.

I also thought using a TaskSupervisor inside a Task would be bad practice. Do you think it’s okay to do this, or could it lead to some issues?

4

Everything is a process including supervisors, their only difference is that they catch exits from their child processes and restarts them, you can decide whether your topology makes sense.

1 Like
5

I also thought using a TaskSupervisor inside a Task would be bad practice. Do you think it’s okay to do this, or could it lead to some issues?

I’m not sure what you mean? Your code can call code in another module at any time, nothing is really “special”.

I think once you start adding a few more tests, you’ll find the parts of this program that could be abstracted a bit, but as far as the code goes, it really is great, for your first shot at a “real OTP program” you’ve done a great job.

There are a few nitpicks I’d say, like rather than cast()-ing to yourself here, I’d use the {:continue, ...} return from init: auto_grader/lib/auto_grader.ex at dev · sheerlox/auto_grader · GitHub

https://hexdocs.pm/elixir/GenServer.html#c:handle_continue/2

You’re also doing a lot to store the parent pid, then send a message back when processing is done, whereas you could use the Task abstraction and just return the value.

If you’re writing use GenServer, restart: :temporary you may instead want a module that use Task instead.

1 Like
6

Before starting this project I thought the TaskSupervisor would be started in my GenServer SubmissionRunner, so that didn’t sound like a good idea to make that module a simple/fragile Task.

I didn’t yet realize at that time the TaskSupervisor is actually started in the application tree (which makes total sense lol). I guess my doubts were based on that false assumption.

I clearly see the benefits of what you’ve pointed, I’ll try to integrate that soon (for science, because I already turned in this year’s grades).

Thanks a lot for the insights!