feat/Policer enhancements #2600
Conversation
carpawell
force-pushed
the
feat/policer-configurable-chill-time
branch
2 times, most recently
from
08d1d8f to
ccbaf27
Compare
Codecov Report
@@ Coverage Diff @@
## master #2600 +/- ##
==========================================
- Coverage 29.75% 29.73% -0.02%
==========================================
Files 407 408 +1
Lines 31161 31215 +54
==========================================
+ Hits 9271 9283 +12
- Misses 21079 21120 +41
- Partials 811 812 +1
... and 2 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
carpawell
force-pushed
the
feat/policer-configurable-chill-time
branch
from
ccbaf27 to
7bf1671
Compare
carpawell
requested review from
roman-khimov and
cthulhu-rider
as code owners
|
Treat almost every commit as a suggestion and feel free to ask, suggest, and discuss. |
carpawell
force-pushed
the
feat/policer-configurable-chill-time
branch
2 times, most recently
from
bf30255 to
ed306c3
Compare
carpawell
force-pushed
the
feat/policer-configurable-chill-time
branch
3 times, most recently
from
f6c5a91 to
577df06
Compare
|
Made new configs public. |
| @@ -94,6 +94,11 @@ apiclient: | |||
|
|
|||
| policer: | |||
| head_timeout: 15s # timeout for the Policer HEAD remote operation | |||
| cache_size: 1000001 # recently-handled objects cache size | |||
| cache_time: 31s # recently-handled objects cache expiration time | |||
There was a problem hiding this comment.
size and time to cache section?
but 1st of all: why do we expose all parameters while #2590 suggests replication_cooldown only?
There was a problem hiding this comment.
size and time to cache section?
well, could be done but i am almost sure the resulting config will change so lets wait testing results and see what we need
There was a problem hiding this comment.
why do we expose all parameters while #2590 suggests replication_cooldown only?
the whole PR is a suggestion. we have magic consts and I am not sure how they work. so now we have a PR that can be tested
| @@ -94,6 +94,11 @@ apiclient: | |||
|
|
|||
| policer: | |||
| head_timeout: 15s # timeout for the Policer HEAD remote operation | |||
| cache_size: 1000001 # recently-handled objects cache size | |||
| cache_time: 31s # recently-handled objects cache expiration time | |||
| replication_cooldown: 101ms # cooldown time b/w replication tasks submitting | |||
There was a problem hiding this comment.
it's implemented like interval, not cooldown, so why is it called so?
There was a problem hiding this comment.
it's implemented like interval, not cooldown
what do you mean here? the current implementation is what i would call a cooldown (but i could be wrong)
There was a problem hiding this comment.
if replication_cooldown is 10s and last replication took 5s, then next replication will start after 5s or 10s? If 10s, then yes, this is cooldown
There was a problem hiding this comment.
lets use some ascii paintings here
Rs -- replication started
Rf -- replication finished
Rs-----Rf-----Rs--------------------Rf/Rs
|______||_____||______________________|
5s 5s 20s
a cooldown to me is "i used some func, next time i will be able to use it no earlier than after *cooldown_time*"
an interval to me is "i used some func, lets sleep for *interval_time*"
|
That was not a mistake, that is how i usually do such things.
The second commit is a summary: now the policer is configurable. The first one is about a policer's internal change (although the defaults work ok and no behavior changes were presented (btw, the commit says that nothing changed)). Dont mind swapping if you think that it is not correct (but it won't be swapping in fact, i will need to add a commit that describes some params and then i would need a commit that describes the cooldown param). |
|
Commits are OK as is. And OK the other way around. It doesn't change much here. |
We decreased it from 200_000 to 1024 in 091448f in order not to have a potentially useless memory consumption (with 30 seconds cache expiration and 200_000 cache size, it looks impossible to HEAD the same object twice for some _real_ disk load). Also, the commit message was wrong about max memory usage. This commit suggests leading the implementation to the comment. Increasing cache size, it is possible to make it more sane for the real situations and, at the same time, 64MB is the maximum object size currently, that should be fitted in memory freely more times more than once. Anyway, 1024 cache size looks incredibly senseless even for the 1024 objects in the storage: 100ms object handling leads to ~100 seconds before object handling repetition. Some other solutions: - cache removing; - cache size that depends on the total available memory; - cache size that depends on the total number of the objects in the storage; - a configurable cache size. Signed-off-by: Pavel Karpy <[email protected]>
Make replication tasks by policer no more than once in a certain period of time. Currently, this period is a zero `time.Duration` so no functional changes for this commit. Refs nspcc-dev#2590. Signed-off-by: Pavel Karpy <[email protected]>
Closes nspcc-dev#2590. Signed-off-by: Pavel Karpy <[email protected]>
Available for reconfiguration fields: - head requests timeout; - recent-handled objects cache size AND expiration time; - replication cooldown time; - max worker pool capacity; - max number of objects read per iteration. Signed-off-by: Pavel Karpy <[email protected]>
10 is not practical at all, even testnet has already more. Signed-off-by: Pavel Karpy <[email protected]>
carpawell
force-pushed
the
feat/policer-configurable-chill-time
branch
from
577df06 to
82e7866
Compare
Internal testing did not prove any valuable cache effect. Signed-off-by: Pavel Karpy <[email protected]>
carpawell
force-pushed
the
feat/policer-configurable-chill-time
branch
from
82e7866 to
40762d3
Compare
No description provided.