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

Add optional handle_enqueue/2 and handle_dequeue/2 callbacks #7

Merged
merged 13 commits into from
Apr 28, 2020

Conversation

sneako
Copy link
Contributor

@sneako sneako commented Apr 26, 2020

Closes #5

@josevalim
Copy link
Member

Hi @sneako! I had an idea, let’s make handle_enqueue(command, pool_state). The item is opaque but you can wrap it, for example to attach some information to be used on dequeue. On dequeue we receive the potentially wrapped item and unwrap it.

This can allow you to do things like having queues based on how long the items are in the queue (sojourn time). We can even improve enqueue/dequeue later to allow items to be skipped if we think that we won’t be able to serve it on time. In fact, we would be able to implement the whole deadline on top of those callbacks.

@sneako sneako marked this pull request as draft April 26, 2020 19:50
@sneako
Copy link
Contributor Author

sneako commented Apr 26, 2020

@josevalim that sounds really interesting! I just pushed an update, am I on the right track here? I think the tests could probably be improved, but I just want to make sure I'm following correctly.

@sneako sneako changed the title Add optional handle_enqueue/1 and handle_dequeue/1 callbacks Add optional handle_enqueue/2 and handle_dequeue/2 callbacks Apr 26, 2020
lib/nimble_pool.ex Outdated Show resolved Hide resolved
lib/nimble_pool.ex Outdated Show resolved Hide resolved
lib/nimble_pool.ex Outdated Show resolved Hide resolved
lib/nimble_pool.ex Outdated Show resolved Hide resolved
@josevalim
Copy link
Member

Hi @sneako, sorry for the back and forth but I think that, after reviewing your changes, we need to bring the deadline in and keep it built-in. Here are the reasons:

  1. If we don't bring the deadline in, we need to send a cancel request from timeout, as you did. But if we do a cancel request from timeout and you implement deadline by hand in Finch, then we will have to implement this clause, as you can skip something and still receive its timeout message

  2. If we skip a message on handle_enqueue/handle_dequeue, we will have to send a message back to the client saying the checkout was skipped. If we don't have a deadline, there is a chance we can send this message after the timeout, polluting the process inbox in case it doesn't terminate

So it seems to me having deadlines will provide a better experience altogether. Can you please bring it back? :) WDYT?

The good news is that we can probably implement it inside the handle_dequeue and handle_enqueue callbacks themselves.

Comment on lines 558 to 559
with {:ok, command, state} <- handle_dequeue(command, state),
{{:value, worker_server_state}, resources} <- :queue.out(resources) do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's please break it in two cases. Otherwise, if someone returns {:empty, state} from dequeue, it will actually work!

Also, we need to first checkout the resource, then we need to dequeue. :) Otherwise we may dequeue and not use it, meaning it would be dequeued multiple times.


receive do
{^ref, :skipped} ->
Process.demonitor(ref, [:flush])
exit!(:skipped, :checkout, [pool])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a big fan of using :skipped here because it doesn't say much. What if we allow {:skip, Exception.t, pool_state} to be returned from the callbacks? Then in here, we simply do raise exception?

@sneako
Copy link
Contributor Author

sneako commented Apr 27, 2020

No apology necessary! This is a huge learning experience for me, so I appreciate you bearing with me as I fumble through this! I agree, having the deadline built in is a much nicer experience

@sneako
Copy link
Contributor Author

sneako commented Apr 28, 2020

Still have to update the docs, and could probably improve the tests a bit, but I think this is ready for another look :)


case apply_worker_callback(worker, :handle_checkout, args) do
{:ok, worker_client_state, worker_server_state} ->
case handle_dequeue(command, deadline, ref, mon_ref, state) do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hrm, we need to call handle_dequeue before handle_checkout, otherwise handle_checkout will receive the wrapped handle_dequeue item. However, if we call it before, then handle_checkout can fail, which means we need to enqueue it again. Damn it. :D

So in a way, it feels like handle_checkout and handle_dequeue are the same callback. Maybe what we need to do is:

  1. Let's remove handle_dequeue for now and merge this PR. In particular, maybe_checkout can look exactly how it was before
  2. Let's pass pool_state to handle_checkout and handle_checkin and require it to be returned in their return types
  3. Support {:skip, exception, pool_state} in handle_checkout

WDTY?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, I think this makes sense. I was already thinking that I would only be using handle enqueue to implement round robin in Finch, and probably not need to implement handle_dequeue. I'll remove handle_dequeue, and update the other callbacks as specified in 2 & 3

state = remove(reason, worker_server_state, state)
state = %{state | requests: requests, monitors: monitors, resources: resources}
{:noreply, state}
state = remove_request(state, ref, mon_ref)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call on remove_request/3!

@sneako sneako marked this pull request as ready for review April 28, 2020 15:08
{:ok, client_state, worker_state} | {:remove, user_reason}
@callback handle_checkout(maybe_wrapped_command :: term, from, worker_state, pool_state) ::
{:ok, client_state, worker_state, pool_state}
| {:remove, user_reason}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. Let's just request the pool_state to be returned on {:remove, user_reason} too and we can ship it. The same for remove on checkin!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm it looks like I will either have to add pool_state to all of the callbacks that can return remove, or define a second apply_worker_callback that accepts the pool state and returns it when it catches. I guess the second option sounds better

@josevalim josevalim merged commit 5a3928a into dashbitco:master Apr 28, 2020
@josevalim
Copy link
Member

💚 💙 💜 💛 ❤️

@sneako sneako deleted the enqueue-dequeue branch April 28, 2020 16:39
@josevalim
Copy link
Member

Hi @sneako ! I failed to notice we were passing more state than we should in my review, so I pushed a fix to master!

@sneako
Copy link
Contributor Author

sneako commented Apr 28, 2020

Ah right that makes sense! We definitely don't want to let the user mess with the other fields. Thanks for the heads up!

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.

Accessing queue length
2 participants