[
https://issues.apache.org/jira/browse/HBASE-13888?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14584580#comment-14584580
]
Ashish Singhi commented on HBASE-13888:
---------------------------------------
Sorry for the delay I was out of the town for last two days.
I am not very clear with your bug scenario, can you explain me in short what
you want to achieve which is failing ?
Regarding the AverageIntervalRateLimiter#refill bug which you are pointing is
not correct to me.
I see that you have written a test case for that,
{code}
@Test
public void testRefillOfAverageIntervalRateLimiter() throws
InterruptedException {
RateLimiter limiter = new AverageIntervalRateLimiter();
limiter.set(60, TimeUnit.SECONDS);
assertEquals(60, limiter.getAvailable());
// first refill, will return the number same with limit
assertEquals(60, limiter.refill(limiter.getLimit(),
limiter.getAvailable()));
limiter.consume(60);
// after 0.2 sec, refill should return 12
limiter.setNextRefillTime(limiter.getNextRefillTime() - 200);
assertEquals(12, limiter.refill(limiter.getLimit(),
limiter.getAvailable()));
// after 0.5 sec, refill should return 30
limiter.setNextRefillTime(limiter.getNextRefillTime() - 500);
assertEquals(30, limiter.refill(limiter.getLimit(),
limiter.getAvailable()));
// after 1 sec, refill should return 60
limiter.setNextRefillTime(limiter.getNextRefillTime() - 1000);
assertEquals(60, limiter.refill(limiter.getLimit(),
limiter.getAvailable()));
// after more than 1 sec, refill should return at max 60
limiter.setNextRefillTime(limiter.getNextRefillTime() - 3000);
assertEquals(60, limiter.refill(limiter.getLimit(),
limiter.getAvailable()));
limiter.setNextRefillTime(limiter.getNextRefillTime() - 5000);
assertEquals(60, limiter.refill(limiter.getLimit(),
limiter.getAvailable()));
}
{code}
here you are not consuming any resource and updating the refill time is not at
all correct which means without consuming any resource by user refill time is
getting updated how is this possible in real world scenario !!?
If we add code to consume all 60 resources before we update the next refill
time then the test will pass without your source code modification also.
Regarding {{testCanExecuteOfAverageIntervalRateLimiter}} I did not understand
what are you testing here, can you explain ?
Thanks.
> refill bug from HBASE-13686
> ---------------------------
>
> Key: HBASE-13888
> URL: https://issues.apache.org/jira/browse/HBASE-13888
> Project: HBase
> Issue Type: Bug
> Affects Versions: 2.0.0
> Reporter: Guanghao Zhang
> Assignee: Guanghao Zhang
> Attachments: HBASE-13888-v1.patch, HBASE-13888-v2.patch
>
>
> As I report the RateLimiter fail to limit in HBASE-13686, then [~ashish
> singhi] fix that problem by support two kinds of RateLimiter:
> AverageIntervalRateLimiter and FixedIntervalRateLimiter. But in my use of the
> code, I found a new bug about refill() in AverageIntervalRateLimiter.
> {code}
> long delta = (limit * (now - nextRefillTime)) /
> super.getTimeUnitInMillis();
> if (delta > 0) {
> this.nextRefillTime = now;
> return Math.min(limit, available + delta);
> }
> {code}
> When delta > 0, refill maybe return available + delta. Then in the
> canExecute(), avail will add refillAmount again. So the new avail maybe 2 *
> avail + delta.
> {code}
> long refillAmount = refill(limit, avail);
> if (refillAmount == 0 && avail < amount) {
> return false;
> }
> // check for positive overflow
> if (avail <= Long.MAX_VALUE - refillAmount) {
> avail = Math.max(0, Math.min(avail + refillAmount, limit));
> } else {
> avail = Math.max(0, limit);
> }
> {code}
> I will add more unit tests for RateLimiter in the next days.
> Review Board: https://reviews.apache.org/r/35384/
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
