hbase-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ashish Singhi (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HBASE-13888) refill bug from HBASE-13686
Date Sat, 13 Jun 2015 12:10:00 GMT

    [ 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)

Mime
View raw message