-
Notifications
You must be signed in to change notification settings - Fork 23
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
Throttles/rework run and sending logic #180
Conversation
f287c38
to
b5123e3
Compare
Notice that a normal send would spawn a local process, send a pid across distribution, have the local process receive an atom across distribution, and have the `Msg' be delivered to the requester locally. That is 1 pid and 1 atom across distribution and one local payload, plus one spawn that copies the payload. Doing a call instead will only send back and forth the payload, and already solve monitoring problems This is specially optimal when the payload is a literal, like an atom.
Note that in this case, the desired operation will never happen, and therefore the test has been deemed invalid.
3d9d72f
to
443ca6b
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #180 +/- ##
==========================================
+ Coverage 73.44% 73.80% +0.36%
==========================================
Files 29 30 +1
Lines 1043 1050 +7
==========================================
+ Hits 766 775 +9
+ Misses 277 275 -2 ☔ View full report in Codecov by Sentry. |
We here revert the changes of the previous commits and instead move the logic regarding the `async_runner` to a separate module. This way, the `amoc_throttle_process`'s only responsibility is to track runners and notify them that they can run; the `amoc_throttle_controller`'s only responsibility is to assign tasks to processes and control the rates; and `amoc_throttle_runner`'s only responsibility is to execute the actions.
With this, now `amoc_throttle_process` only tracks the runners and doesn't know about creating them nor about what operation is it running, and the controller now only controls the rate, and not the choosing of a worker.
With this, now `amoc_throttle_runner` optimises the different use-cases of the throttling API: executing arbitrary code, sending a payload, or blocking the caller.
b9971d5
to
1e25a29
Compare
@@ -35,8 +37,8 @@ | |||
interval = 0 :: amoc_throttle:interval(), %%ms | |||
delay_between_executions = 0 :: non_neg_integer(), %%ms | |||
tref :: timer:tref() | undefined, | |||
schedule = [] :: [pid()], | |||
schedule_reversed = [] :: [pid()]}). | |||
schedule = [] :: [amoc_throttle_runner:runner()], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
avoid introduction of unneeded types. it is pid(), it has to be pid() and it cannot be anything else. since we set up process monitoring with erlang:monitor/2 for the items in this list. if you want to highlight that it's a process created by amoc_trottle_runner, then the spec could look like [AmocTrottleRunnerProcess::pid()]
|
||
-spec run(runner()) -> reference(). | ||
run(RunnerPid) -> | ||
Ref = erlang:monitor(process, RunnerPid), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
by the time this function is executed, the monitoring is already set up. 2 monitors is definetly a bug, please remove this call.
since amoc_throttle_process relies on monitor messages I would expect it to explicitely create that monitors
0798c8a
to
42050d5
Compare
test/throttle_SUITE.erl
Outdated
wait_until_one_throttle_worker(?FUNCTION_NAME), | ||
amoc_throttle:send(?FUNCTION_NAME, receive_this), | ||
wait_until_one_async_runner(self(), OriginalLinks), | ||
kill_throttle_workers(?FUNCTION_NAME), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can just stop the trottle with amoc_trottle:stop/1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, true :D
42050d5
to
5060d89
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me.
An attempt to tackle the issue raised in #178 (comment)
Introduces a new API endpoint called
wait
that supersedessend_and_wait
, as the latter was redundant and could possibly result in a deadlock was the throttling process to die.