Interarrival as throttle #186
Conversation
NelsonVides
force-pushed
the
interarrival_as_throttle
branch
from
a5f279b to
42d65c5
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #186 +/- ##
==========================================
+ Coverage 75.83% 77.20% +1.37%
==========================================
Files 32 32
Lines 1200 1167 -33
==========================================
- Hits 910 901 -9
+ Misses 290 266 -24 ☔ View full report in Codecov by Sentry. |
NelsonVides
force-pushed
the
throttle_api
branch
2 times, most recently
from
c16c3d6 to
2ae0719
Compare
NelsonVides
force-pushed
the
throttle_api
branch
16 times, most recently
from
73f1e61 to
92ef315
Compare
This also very importantly removes timers and loops from the critical controller process.
NelsonVides
force-pushed
the
interarrival_as_throttle
branch
from
58f94c0 to
416b25e
Compare
NelsonVides
force-pushed
the
interarrival_as_throttle
branch
from
416b25e to
8886183
Compare
| non_neg_integer(Interarrival) -> | ||
| is_integer(Interarrival) andalso Interarrival >= 0. | ||
| -spec verify_user_rate(any()) -> boolean(). | ||
| verify_user_rate(UserRate) -> |
There was a problem hiding this comment.
please use braces to make expression more readable
There was a problem hiding this comment.
(is_integer(UserRate) andalso (0 =< UserRate))
orelse (infinity =:= UserRate)
| @@ -143,19 +143,13 @@ maybe_stop_timer(#state{tref = undefined}) -> | |||
| ok; | |||
| maybe_stop_timer(#state{tref = TRef}) -> | |||
| {ok, cancel} = timer:cancel(TRef), | |||
| consume_all_timer_ticks(delay_between_executions). | |||
| amoc_throttle_controller:consume_all_messages(delay_between_executions). | |||
There was a problem hiding this comment.
revert this change, it introduces erronous impression that we do here something with amoc_throttle_controller process.
There was a problem hiding this comment.
So where would you put it? The code was duplicated, wanted to put it in some common helper.
There was a problem hiding this comment.
if we don't have any suitable helper module for this, then I would rather want to duplicate code.
| for two consecutive users: | ||
| * default value - 50 ms. | ||
| * example: `AMOC_INTERARRIVAL="50"` | ||
| * `user_rate` - a rate (implemented as a `t:amoc_throttle:rate/0`) of user processes per minute: |
There was a problem hiding this comment.
what is t:amoc_throttle:rate/0?
| @@ -4,7 +4,7 @@ | |||
| %% | |||
| %% It spawns a pool of workers as big as online schedulers. When starting a new user, as the user is | |||
| %% identified by ID, a worker will be chosen for this user based on its ID | |||
| %% (see get_sup_for_user_id/1). | |||
| %% (see `gen_sup_from_userid/1'). | |||
There was a problem hiding this comment.
why gen_sup_from_userid/1? the old name seems to be more relevant.
| Assignments = assign_users_to_sups(SupCount, Supervisors, UserIds, Acc), | ||
| -spec start_children(amoc:scenario(), {amoc_scenario:user_id(), amoc_scenario:user_id()}, any()) -> | ||
| ok. | ||
| start_children(Scenario, {StartId, EndId}, ScenarioState) -> |
There was a problem hiding this comment.
let's convert StartId, EndId into normal function parameters, tuple is unneeded here.
| incr_no_of_users(SupNum) when SupNum > 1 -> | ||
| -spec handle_up_user(non_neg_integer(), pid(), amoc_scenario:user_id()) -> any(). | ||
| handle_up_user(SupNum, Pid, Id) when SupNum > 1 -> | ||
| ets:insert(?TABLE, {Pid, Id}), |
There was a problem hiding this comment.
can we just put user_id into the process dictionary of the user process? and add an interface at amoc_user to get user_id by pid (must be an rpc call for remote processes since process_info interface works only for the local processes)
| %% Supervisor | ||
|
|
||
| %% @private | ||
| -spec start_link() -> supervisor:startlink_ret(). | ||
| start_link() -> | ||
| Ret = supervisor:start_link({local, ?MODULE}, ?MODULE, no_args), | ||
| UserSups = supervisor:which_children(?MODULE), | ||
| IndexedSupsUnsorted = [ {Pid, N} || {{amoc_users_worker_sup, N}, Pid, _, _} <- UserSups], | ||
| IndexedSupsUnsorted = [ {Pid, N} || {{amoc_users_worker_sup, N}, Pid, _, _} <- UserSups, |
There was a problem hiding this comment.
could you remind me why it's unsorted?
There was a problem hiding this comment.
and please add a comment at indexes/0 that we start indexes from 2 to simplify user_count atomics management.
| %% Supervisor | ||
|
|
||
| %% @private | ||
| -spec start_link() -> supervisor:startlink_ret(). | ||
| start_link() -> | ||
| Ret = supervisor:start_link({local, ?MODULE}, ?MODULE, no_args), | ||
| UserSups = supervisor:which_children(?MODULE), | ||
| IndexedSupsUnsorted = [ {Pid, N} || {{amoc_users_worker_sup, N}, Pid, _, _} <- UserSups], | ||
| IndexedSupsUnsorted = [ {Pid, N} || {{amoc_users_worker_sup, N}, Pid, _, _} <- UserSups, | ||
| is_integer(N), is_pid(Pid)], |
There was a problem hiding this comment.
how is it possible that these filters could return false?
| Objects | ||
| end, | ||
| stop_children_assignments(Users, Force), | ||
| length(Users). | ||
|
|
||
| -spec get_all_children() -> [{pid(), amoc_scenario:user_id()}]. | ||
| get_all_children() -> |
There was a problem hiding this comment.
seems that this function is used for testing only, we can remove it and rework testing a bit.
| interarrival_equal_zero_starts_all_users_at_once(_) -> | ||
| Vars = [{interarrival, 0}, {testing_var1, def1} | test_helpers:other_vars_to_keep_quiet()], | ||
| user_rate_equal_zero_starts_all_users_at_once(_) -> | ||
| Vars = [{user_rate, 0}, {testing_var1, def1} |
There was a problem hiding this comment.
it must be infinity user rate, not 0.
No description provided.