HBASE-29912 Codel lifoThreshold should be applied on soft queue limit…#7864
HBASE-29912 Codel lifoThreshold should be applied on soft queue limit…#7864Umeshkumar9414 wants to merge 7 commits into
Conversation
|
|
||
| // so we can calculate actual threshold to switch to LIFO under load | ||
| private int maxCapacity; | ||
| private int currentQueueLimit; |
There was a problem hiding this comment.
Because this can be updated with onConfigurationChange should this be volatile ? Just a nit.
There was a problem hiding this comment.
This is also confusing in that it seems to shadow a variable of the same name that is declared volatile?
Please don't shadow. Why do we need this separately?
There was a problem hiding this comment.
I made currentQueueLimit volatile.
Please don't shadow. Why do we need this separately?
Generally a queue should only handle hard limit and layer above it can handle soft limit and that is what was happening but for codel, we needed queue to know the soft limit as well, I didn't saw a way to use the same variable at both place.
There was a problem hiding this comment.
At least let's not shadow the parent class's variable with the same name. That is a code smell. Pick another one.
Umeshkumar9414
force-pushed
the
HBASE-29912
branch
from
34cb3fa to
ba1bb2c
Compare
|
Yetus JDK17 Hadoop3 Unit check failed on TestNamespaceReplication Test case with timout, I checked on my local, it is passing. No related change in this PR as well. Looks like a flakey. |
Umeshkumar9414
force-pushed
the
HBASE-29912
branch
from
ba1bb2c to
572aa68
Compare
|
cc @saintstack |
apurtell
left a comment
apurtell
left a comment
There was a problem hiding this comment.
Minor changes needed. Otherwise lgtm.
Umeshkumar9414
force-pushed
the
HBASE-29912
branch
from
572aa68 to
da783dd
Compare
Umeshkumar9414
force-pushed
the
HBASE-29912
branch
from
da783dd to
d0956b6
Compare
apurtell
left a comment
apurtell
left a comment
There was a problem hiding this comment.
Everything else looks good, but the new test will probably flake.
Umeshkumar9414
force-pushed
the
HBASE-29912
branch
from
d0956b6 to
eed5bca
Compare
… instead of hard limit
Umeshkumar9414
force-pushed
the
HBASE-29912
branch
from
eed5bca to
3c77fe9
Compare
|
For branch-2.5 #8346 |
2 participants
… instead of hard limit
I think that after HBASE-16089 codel target delay was changed to 100, that is not standard for CoDel (patch). I have changed it back.
To visualize why we should have 5 target delay with 100 interval, I used the model, With target delay 100 and interval 100 we are more prone the queue being full, although CoDel will be still helpful in LIFO mode but a full queue is problem as it can cause OOM.