fixed_count: ability to spawn a specific number of users (as opposed to just using weights)#1964
Conversation
|
We should probably mention it somewhere in the documentation. @mboutet knows most about dispatching - what do you think? |
cyberw
changed the title
|
I made the changes about which I wrote, it seems to me that it turned out quite appropriate. I made two separate functions for counting the ratio of classes and tasks, as can be seen from unit tests, the results of the calculation remained the same, only a little changed approach. |
|
@cyberw Should I do something else or request ready to review now? |
|
Add a mention at some appropriate point in the docs (.rst files). I’m hoping @mboutet will review so I dont have to :) |
|
I'll try to look at this first week of January. Sorry for the delay. |
|
|
||
| self.case_handler( | ||
| cases=[ | ||
| self.Case(fixed_counts=(1, 1), weights=(1, 1, 1), target_user_count=5), |
There was a problem hiding this comment.
Do the tests validat the order in which users are spawned or only final distribution?
There was a problem hiding this comment.
Only final distribution. Since the same algorithm is used as for weighted users, and it is tested above.
| user_class.fixed_count, | ||
| ) | ||
|
|
||
|
|
There was a problem hiding this comment.
Add an ”integration test” in test_main to test some basic functionality using headless.
There was a problem hiding this comment.
I've looked into test_main and a little bit confused, what exactly you want to test? Basic spawning with fixed users? ie check the output is ok and corresponds to the users data?
|
I think adding an integration test similar to locust/locust/test/test_runners.py Line 957 in cce0d29 would be good. |
| self._try_dispatch_fixed = True | ||
|
|
||
| self._no_user_to_spawn = False |
There was a problem hiding this comment.
There was a problem hiding this comment.
I guess no, self._no_user_to_spawn resets to False almost immediately after checking in _dispatcher
self._try_dispatch_fixed will be True if we stop to spawn somewhere on fixed users, so when we continue it is fine. If we spawned all fixed and it is False, it is also ok. The only case we need to reset it is ramp-down. Also I guess nothing changes if to set this fields, except a superfluous work in _try_dispatch_fixed if it was False. But I do not want to mislead about such a need.
|
Can you also add a small stress test like locust/locust/test/test_dispatch.py Line 1934 in cce0d29 ? This is just to make sure there's no significant performance regression. |
|
I'll have to take another look later, but it looks good at first glance. The unit tests you added in |
EzR1d3r
force-pushed
the
issue-1939
branch
2 times, most recently
from
d744faf to
14212c3
Compare
|
I made some fixups, also will add the tests @mboutet asked. |
|
@EzR1d3r, the PR looks good. The tests in I also tested your changes with the scenario in
After the last comments are resolved, I think this will be good to go. |
|
It was a good idea to add TestLargeScale, so I found a bug with _distribute_users function and I had to remake the realisation of fixed users spawning. Within the _distribute_users iterating over the user_gen not add the users in _users_on_workers instantly and we cant monitore the count of the fixed users. Also I a little bit reduced the test code and added my test into existed. This changes in the new last commit. |
EzR1d3r
force-pushed
the
issue-1939
branch
2 times, most recently
from
a3af278 to
07ef480
Compare
|
I see the tests fail. But my locals tests are ok. upd.: in some reason, is missed the tail of "Shutting down (exit code 0), bye." |
I recently removed the ”, bye.” in master. Do thr same in your test and you should be fine. Worthless timing by me… |
for spawning of each class with <fixed_count> class field. Fixed users spawn first. The weight parameter for them is ignored. Also added unit-tests for cases with fixed users.
|
Looks pretty good to me. Just needs an entry somewhere in the documentation (.rst files). One question, why did you add the _actual in the method names? Is that the right word? What would be the ”not actual” ratio? Maybe _current is more appropriate? (on mobile right now, sorry if it would have become obvious by reading the whole PR at once) Anything left to do in your view, @mboutet ? |
I will look.
I guess, you right. 'Actual' in relation to previous 'static' behaviour. But if you dont know the previous, it has no sense, right? |
Remove sounds good! |
…atio, now it is considered based on the real number of users. On the Task tab (web ui) the data is updated every 1 second, so it is possible to see the actual ratio changing. For command-line arguments --show-task-ratio, --show-task-ratio-json, the behavior is also changed - the ratio pre-calculation based on passed num_users. If there is no fixed_count users and num_users argument is None, the old behaviour occurs.
|
Added some docs (last commit) and removed 'actual'. |
Because in case then UserDispatcher._users_on_workers is not fills instantly (see UserDispatcher._distribute_users) we cant to monitore actual count of each user. Also added some additional tests, includes to check this behaviour.
|
Awesome stuff, thanks! This will automatically become part of the next prerelease build, and I'll do a real release in a couple days. |
|
Thx you too for review and accepting this feature! |
|
I don't believe independant load test shapes has been implemented. |
|
No, independent load shapes is not implemented. That ticket is closed because it was stale. Dont know why it was referenced.. I guess in some cases fixed user count helps a little. |
|
I guess I assumed wrongly scope of this feature. |
4 participants
Issue #1939
Here is the overall working option, but noticed a problem with the display 'Ratio per User class' and 'Total ratio' on web UI.
I'm going to redo the logic a little bit, get_task_ratio_dict it will recalculate the ratio based on the number of users at the moment. I'll make it a separate commit in same branch. Maybe there are other ideas?