You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@dubbo.apache.org by Imteyaz Khan <kh...@gmail.com> on 2018/12/19 11:42:18 UTC
ActiveLimitFilter some observation! Need your suggestion
Hi All,
I was trying to write UT for ActiveLimitFilter for RuntimeException
scenarion and below is my UT for the same
ActiveLimitFilterTest.java
@Test()
public void testInvokeRuntimeExceptionWithActiveCountMatch() {
URL url = URL.valueOf("test://test:11/test?accesslog=true&group=dubbo&version=1.1&actives=0");
Invoker<ActiveLimitFilterTest> invoker = new RuntimeExceptionInvoker(url);
Invocation invocation = new MockInvocation();
RpcStatus count = RpcStatus.getStatus(invoker.getUrl(),
invocation.getMethodName());
int beforeExceptionActiveCount = count.getActive();
try {
activeLimitFilter.invoke(invoker, invocation);
} catch (RuntimeException ex) {
int afterExceptionActiveCount = count.getActive();
assertEquals("After exception active count should be same"
, beforeExceptionActiveCount, afterExceptionActiveCount);
}
}
Where I am expecting RpcStatus active count before call and after invoke
should be same, irrespective of exceptional handling by ActiveLimitFilter
(e.g. in this case it should be 0). UT showing me that after encountering
exception it is not same, on my further investigation I found that
- If there is no *actives (ACTIVE_KEY) *is set or if its value is less
then *1* then it is always returning -1 (in UT active count), which
means there is more number of call can be possible then it is allowed(this
is my interpretation , correct me if this is wrong). e.g. if within a
minute 10 call are allowed and if we encounter 5 *RuntimeException *then
in total we could landed up allowing 15 invoke. I am suspecting this is
because *max *being *0* we don't increment active count of RpcStatus
and then decrease it in finally block where we have not even have increase
the count, because *max* is before the count increment
if (max > 0 && !RpcStatus.beginCount(url, methodName, max))
As a reference I am copying the current master branch code of
ActiveLimitFilter.java. Would request to correct my understanding if it is
wrong, and if you feel my observation is correct then I would can raise a
PR for fixing this issue.
*Note: UT is in my local I have not checked in into any branch.
if (max > 0 && !RpcStatus.beginCount(url, methodName, max)) {
long timeout =
invoker.getUrl().getMethodParameter(invocation.getMethodName(),
Constants.TIMEOUT_KEY, 0);
long start = System.currentTimeMillis();
long remain = timeout;
synchronized (count) {
while (!RpcStatus.beginCount(url, methodName, max)) {
try {
count.wait(remain);
} catch (InterruptedException e) {
// ignore
}
long elapsed = System.currentTimeMillis() - start;
remain = timeout - elapsed;
if (remain <= 0) {
throw new RpcException("Waiting concurrent
invoke timeout in client-side for service: "
+ invoker.getInterface().getName() +
", method: "
+ invocation.getMethodName() + ",
elapsed: " + elapsed
+ ", timeout: " + timeout + ".
concurrent invokes: " + count.getActive()
+ ". max concurrent invoke limit: " + max);
}
}
}
}
boolean isSuccess = true;
long begin = System.currentTimeMillis();
try {
return invoker.invoke(invocation);
} catch (RuntimeException t) {
isSuccess = false;
throw t;
} finally {
RpcStatus.endCount(url, methodName,
System.currentTimeMillis() - begin, isSuccess);
if (max > 0) {
synchronized (count) {
count.notifyAll();
}
}
}
Re: ActiveLimitFilter some observation! Need your suggestion
Posted by Ian Luo <ia...@gmail.com>.
We cannot do this, beginCount and endCount need to be called when max == 0
too.
Thanks,
-Ian.
On Fri, Dec 21, 2018 at 1:12 AM 大步流星 <83...@qq.com> wrote:
> 能不能这样：
> Can you do this:
>
>
> } finally {
> RpcStatus.endCount(url, methodName, System.currentTimeMillis()
> - begin, isSuccess);
> if (max > 0) {
> synchronized (count) {
> count.notifyAll();
> }
> }
> }
>
> Change to the following:
>
> } finally {
> if (max > 0) {
> RpcStatus.endCount(url, methodName,
> System.currentTimeMillis() - begin, isSuccess);
> synchronized (count) {
> count.notifyAll();
> }
> }
> }
>
>
>
> ------------------ 原始邮件 ------------------
> 发件人: "Ian Luo"<ia...@gmail.com>;
> 发送时间: 2018年12月20日(星期四) 上午10:11
> 收件人: "dev"<de...@dubbo.apache.org>;
>
> 主题: Re: ActiveLimitFilter some observation! Need your suggestion
>
>
>
> We cannot simply comment out 'endCount' in finally clause. If 'beginCount'
> happens, then 'endCount' must be called.
>
> Thanks,
> -Ian.
>
> On Wed, Dec 19, 2018 at 9:49 PM Imteyaz Khan <kh...@gmail.com>
> wrote:
>
> > Ian,
> > The approach you mentioned, I am certain it would works. I was thinking
> > another approach
> >
> > if (!RpcStatus.beginCount(url, methodName, max) && max > 0) {
> > }
> >
> > ...
> > finally {
> > //already existing endCount call.
> > }
> >
> > Second approach I am mentioning because of simplification. Do let me know
> > your thoughts on this.
> >
> >
> > On Wed, Dec 19, 2018 at 6:39 PM Ian Luo <ia...@gmail.com> wrote:
> >
> > > Hi Khan,
> > >
> > > Thanks for pointing out this issue. I guess we could change logic to:
> > >
> > > if (max > 0 && !RpcStatus.beginCount(url, methodName, max)) {
> > > ...
> > > } else {
> > > RpcStatus.beginCount(url, methodName);
> > > }
> > >
> > > And we should do the same logic in ExecuteLimitFilter. Let me know your
> > > opinion on this.
> > >
> > > Thanks,
> > > -Ian.
> > >
> > >
> > > On Wed, Dec 19, 2018 at 7:42 PM Imteyaz Khan <kh...@gmail.com>
> > > wrote:
> > >
> > > > Hi All,
> > > > I was trying to write UT for ActiveLimitFilter for
> RuntimeException
> > > > scenarion and below is my UT for the same
> > > >
> > > > ActiveLimitFilterTest.java
> > > >
> > > > @Test()
> > > > public void testInvokeRuntimeExceptionWithActiveCountMatch() {
> > > > URL url =
> > > >
> > >
> >
> URL.valueOf("test://test:11/test?accesslog=true&group=dubbo&version=1.1&actives=0");
> > > > Invoker<ActiveLimitFilterTest> invoker = new
> > > > RuntimeExceptionInvoker(url);
> > > > Invocation invocation = new MockInvocation();
> > > > RpcStatus count = RpcStatus.getStatus(invoker.getUrl(),
> > > > invocation.getMethodName());
> > > > int beforeExceptionActiveCount = count.getActive();
> > > > try {
> > > > activeLimitFilter.invoke(invoker, invocation);
> > > > } catch (RuntimeException ex) {
> > > > int afterExceptionActiveCount = count.getActive();
> > > > assertEquals("After exception active count should be same"
> > > > , beforeExceptionActiveCount,
> > afterExceptionActiveCount);
> > > > }
> > > > }
> > > >
> > > >
> > > > Where I am expecting RpcStatus active count before call and after
> > invoke
> > > > should be same, irrespective of exceptional handling by
> > ActiveLimitFilter
> > > > (e.g. in this case it should be 0). UT showing me that after
> > encountering
> > > > exception it is not same, on my further investigation I found that
> > > >
> > > >
> > > > - If there is no *actives (ACTIVE_KEY) *is set or if its value is
> > less
> > > > then *1* then it is always returning -1 (in UT active count),
> which
> > > > means there is more number of call can be possible then it is
> > > > allowed(this
> > > > is my interpretation , correct me if this is wrong). e.g. if
> within
> > a
> > > > minute 10 call are allowed and if we encounter 5 *RuntimeException
> > > *then
> > > > in total we could landed up allowing 15 invoke. I am suspecting
> this
> > > is
> > > > because *max *being *0* we don't increment active count of
> > RpcStatus
> > > > and then decrease it in finally block where we have not even have
> > > > increase
> > > > the count, because *max* is before the count increment
> > > >
> > > > if (max > 0 && !RpcStatus.beginCount(url, methodName,
> > > max))
> > > >
> > > > As a reference I am copying the current master branch code of
> > > > ActiveLimitFilter.java. Would request to correct my understanding if
> it
> > > is
> > > > wrong, and if you feel my observation is correct then I would can
> > raise a
> > > > PR for fixing this issue.
> > > > *Note: UT is in my local I have not checked in into any branch.
> > > >
> > > > if (max > 0 && !RpcStatus.beginCount(url, methodName, max)) {
> > > > long timeout =
> > > > invoker.getUrl().getMethodParameter(invocation.getMethodName(),
> > > > Constants.TIMEOUT_KEY, 0);
> > > > long start = System.currentTimeMillis();
> > > > long remain = timeout;
> > > > synchronized (count) {
> > > > while (!RpcStatus.beginCount(url, methodName, max)) {
> > > > try {
> > > > count.wait(remain);
> > > > } catch (InterruptedException e) {
> > > > // ignore
> > > > }
> > > > long elapsed = System.currentTimeMillis() -
> start;
> > > > remain = timeout - elapsed;
> > > > if (remain <= 0) {
> > > > throw new RpcException("Waiting concurrent
> > > > invoke timeout in client-side for service: "
> > > > + invoker.getInterface().getName() +
> > > > ", method: "
> > > > + invocation.getMethodName() + ",
> > > > elapsed: " + elapsed
> > > > + ", timeout: " + timeout + ".
> > > > concurrent invokes: " + count.getActive()
> > > > + ". max concurrent invoke limit: " +
> > > max);
> > > > }
> > > > }
> > > > }
> > > > }
> > > >
> > > > boolean isSuccess = true;
> > > > long begin = System.currentTimeMillis();
> > > > try {
> > > > return invoker.invoke(invocation);
> > > > } catch (RuntimeException t) {
> > > > isSuccess = false;
> > > > throw t;
> > > > } finally {
> > > > RpcStatus.endCount(url, methodName,
> > > > System.currentTimeMillis() - begin, isSuccess);
> > > > if (max > 0) {
> > > > synchronized (count) {
> > > > count.notifyAll();
> > > > }
> > > > }
> > > > }
> > > >
> > >
> >
回复： ActiveLimitFilter some observation! Need your suggestion
Posted by 大步流星 <83...@qq.com>.
能不能这样：
Can you do this:
} finally {
RpcStatus.endCount(url, methodName, System.currentTimeMillis() - begin, isSuccess);
if (max > 0) {
synchronized (count) {
count.notifyAll();
}
}
}
Change to the following:
} finally {
if (max > 0) {
RpcStatus.endCount(url, methodName, System.currentTimeMillis() - begin, isSuccess);
synchronized (count) {
count.notifyAll();
}
}
}
------------------ 原始邮件 ------------------
发件人: "Ian Luo"<ia...@gmail.com>;
发送时间: 2018年12月20日(星期四) 上午10:11
收件人: "dev"<de...@dubbo.apache.org>;
主题: Re: ActiveLimitFilter some observation! Need your suggestion
We cannot simply comment out 'endCount' in finally clause. If 'beginCount'
happens, then 'endCount' must be called.
Thanks,
-Ian.
On Wed, Dec 19, 2018 at 9:49 PM Imteyaz Khan <kh...@gmail.com> wrote:
> Ian,
> The approach you mentioned, I am certain it would works. I was thinking
> another approach
>
> if (!RpcStatus.beginCount(url, methodName, max) && max > 0) {
> }
>
> ...
> finally {
> //already existing endCount call.
> }
>
> Second approach I am mentioning because of simplification. Do let me know
> your thoughts on this.
>
>
> On Wed, Dec 19, 2018 at 6:39 PM Ian Luo <ia...@gmail.com> wrote:
>
> > Hi Khan,
> >
> > Thanks for pointing out this issue. I guess we could change logic to:
> >
> > if (max > 0 && !RpcStatus.beginCount(url, methodName, max)) {
> > ...
> > } else {
> > RpcStatus.beginCount(url, methodName);
> > }
> >
> > And we should do the same logic in ExecuteLimitFilter. Let me know your
> > opinion on this.
> >
> > Thanks,
> > -Ian.
> >
> >
> > On Wed, Dec 19, 2018 at 7:42 PM Imteyaz Khan <kh...@gmail.com>
> > wrote:
> >
> > > Hi All,
> > > I was trying to write UT for ActiveLimitFilter for RuntimeException
> > > scenarion and below is my UT for the same
> > >
> > > ActiveLimitFilterTest.java
> > >
> > > @Test()
> > > public void testInvokeRuntimeExceptionWithActiveCountMatch() {
> > > URL url =
> > >
> >
> URL.valueOf("test://test:11/test?accesslog=true&group=dubbo&version=1.1&actives=0");
> > > Invoker<ActiveLimitFilterTest> invoker = new
> > > RuntimeExceptionInvoker(url);
> > > Invocation invocation = new MockInvocation();
> > > RpcStatus count = RpcStatus.getStatus(invoker.getUrl(),
> > > invocation.getMethodName());
> > > int beforeExceptionActiveCount = count.getActive();
> > > try {
> > > activeLimitFilter.invoke(invoker, invocation);
> > > } catch (RuntimeException ex) {
> > > int afterExceptionActiveCount = count.getActive();
> > > assertEquals("After exception active count should be same"
> > > , beforeExceptionActiveCount,
> afterExceptionActiveCount);
> > > }
> > > }
> > >
> > >
> > > Where I am expecting RpcStatus active count before call and after
> invoke
> > > should be same, irrespective of exceptional handling by
> ActiveLimitFilter
> > > (e.g. in this case it should be 0). UT showing me that after
> encountering
> > > exception it is not same, on my further investigation I found that
> > >
> > >
> > > - If there is no *actives (ACTIVE_KEY) *is set or if its value is
> less
> > > then *1* then it is always returning -1 (in UT active count), which
> > > means there is more number of call can be possible then it is
> > > allowed(this
> > > is my interpretation , correct me if this is wrong). e.g. if within
> a
> > > minute 10 call are allowed and if we encounter 5 *RuntimeException
> > *then
> > > in total we could landed up allowing 15 invoke. I am suspecting this
> > is
> > > because *max *being *0* we don't increment active count of
> RpcStatus
> > > and then decrease it in finally block where we have not even have
> > > increase
> > > the count, because *max* is before the count increment
> > >
> > > if (max > 0 && !RpcStatus.beginCount(url, methodName,
> > max))
> > >
> > > As a reference I am copying the current master branch code of
> > > ActiveLimitFilter.java. Would request to correct my understanding if it
> > is
> > > wrong, and if you feel my observation is correct then I would can
> raise a
> > > PR for fixing this issue.
> > > *Note: UT is in my local I have not checked in into any branch.
> > >
> > > if (max > 0 && !RpcStatus.beginCount(url, methodName, max)) {
> > > long timeout =
> > > invoker.getUrl().getMethodParameter(invocation.getMethodName(),
> > > Constants.TIMEOUT_KEY, 0);
> > > long start = System.currentTimeMillis();
> > > long remain = timeout;
> > > synchronized (count) {
> > > while (!RpcStatus.beginCount(url, methodName, max)) {
> > > try {
> > > count.wait(remain);
> > > } catch (InterruptedException e) {
> > > // ignore
> > > }
> > > long elapsed = System.currentTimeMillis() - start;
> > > remain = timeout - elapsed;
> > > if (remain <= 0) {
> > > throw new RpcException("Waiting concurrent
> > > invoke timeout in client-side for service: "
> > > + invoker.getInterface().getName() +
> > > ", method: "
> > > + invocation.getMethodName() + ",
> > > elapsed: " + elapsed
> > > + ", timeout: " + timeout + ".
> > > concurrent invokes: " + count.getActive()
> > > + ". max concurrent invoke limit: " +
> > max);
> > > }
> > > }
> > > }
> > > }
> > >
> > > boolean isSuccess = true;
> > > long begin = System.currentTimeMillis();
> > > try {
> > > return invoker.invoke(invocation);
> > > } catch (RuntimeException t) {
> > > isSuccess = false;
> > > throw t;
> > > } finally {
> > > RpcStatus.endCount(url, methodName,
> > > System.currentTimeMillis() - begin, isSuccess);
> > > if (max > 0) {
> > > synchronized (count) {
> > > count.notifyAll();
> > > }
> > > }
> > > }
> > >
> >
>
Re: ActiveLimitFilter some observation! Need your suggestion
Posted by Imteyaz Khan <kh...@gmail.com>.
Ian,
Sure, on it.
On Fri, Dec 21, 2018 at 8:34 AM Ian Luo <ia...@gmail.com> wrote:
> Imteyaz,
>
> Would you mind to take a look at pull request 3035 [1] I submitted just
> now?
>
> Thanks,
> -Ian.
>
>
> 1. https://github.com/apache/incubator-dubbo/pull/3035
>
>
> On Thu, Dec 20, 2018 at 4:41 PM Imteyaz Khan <kh...@gmail.com>
> wrote:
>
> > Ian,
> > Yes please, incorporate the fix. Will wait for your PR to review.
> >
> >
> >
> > On Thu, Dec 20, 2018 at 2:01 PM Imteyaz Khan <kh...@gmail.com>
> > wrote:
> >
> > > Ian,
> > > You won't believe, but what a coincidence I just loggined to gmail, to
> > > reply on this tread to say, can I assist you on this issue any way and
> > saw
> > > your email. Any thing is fine with me.
> > >
> > > Regarding the apporach, as you mentioned endCount needs to be called I
> > > certinly agree with you, but I was more looking for simplified approach
> > of
> > > changing if condition check where first check using beginCount then
> > > followed by max>0. In this case, begin count will be always set and
> > before
> > > the end of method endCount will reduce it.
> > >
> > > Please correct me if I am wrong here.
> > >
> > > On Thu, Dec 20, 2018 at 1:54 PM Ian Luo <ia...@gmail.com> wrote:
> > >
> > >> Khan,
> > >>
> > >> I know you may busy on other stuff, should I jump on to give it a fix?
> > >>
> > >> Thanks,
> > >> -Ian.
> > >>
> > >>
> > >> On Thu, Dec 20, 2018 at 10:11 AM Ian Luo <ia...@gmail.com> wrote:
> > >>
> > >> > We cannot simply comment out 'endCount' in finally clause. If
> > >> 'beginCount'
> > >> > happens, then 'endCount' must be called.
> > >> >
> > >> > Thanks,
> > >> > -Ian.
> > >> >
> > >> > On Wed, Dec 19, 2018 at 9:49 PM Imteyaz Khan <
> khan.imteyaz@gmail.com>
> > >> > wrote:
> > >> >
> > >> >> Ian,
> > >> >> The approach you mentioned, I am certain it would works. I was
> > >> thinking
> > >> >> another approach
> > >> >>
> > >> >> if (!RpcStatus.beginCount(url, methodName, max) && max > 0) {
> > >> >> }
> > >> >>
> > >> >> ...
> > >> >> finally {
> > >> >> //already existing endCount call.
> > >> >> }
> > >> >>
> > >> >> Second approach I am mentioning because of simplification. Do let
> me
> > >> know
> > >> >> your thoughts on this.
> > >> >>
> > >> >>
> > >> >> On Wed, Dec 19, 2018 at 6:39 PM Ian Luo <ia...@gmail.com> wrote:
> > >> >>
> > >> >> > Hi Khan,
> > >> >> >
> > >> >> > Thanks for pointing out this issue. I guess we could change logic
> > to:
> > >> >> >
> > >> >> > if (max > 0 && !RpcStatus.beginCount(url, methodName, max)) {
> > >> >> > ...
> > >> >> > } else {
> > >> >> > RpcStatus.beginCount(url, methodName);
> > >> >> > }
> > >> >> >
> > >> >> > And we should do the same logic in ExecuteLimitFilter. Let me
> know
> > >> your
> > >> >> > opinion on this.
> > >> >> >
> > >> >> > Thanks,
> > >> >> > -Ian.
> > >> >> >
> > >> >> >
> > >> >> > On Wed, Dec 19, 2018 at 7:42 PM Imteyaz Khan <
> > khan.imteyaz@gmail.com
> > >> >
> > >> >> > wrote:
> > >> >> >
> > >> >> > > Hi All,
> > >> >> > > I was trying to write UT for ActiveLimitFilter for
> > >> RuntimeException
> > >> >> > > scenarion and below is my UT for the same
> > >> >> > >
> > >> >> > > ActiveLimitFilterTest.java
> > >> >> > >
> > >> >> > > @Test()
> > >> >> > > public void testInvokeRuntimeExceptionWithActiveCountMatch() {
> > >> >> > > URL url =
> > >> >> > >
> > >> >> >
> > >> >>
> > >>
> >
> URL.valueOf("test://test:11/test?accesslog=true&group=dubbo&version=1.1&actives=0");
> > >> >> > > Invoker<ActiveLimitFilterTest> invoker = new
> > >> >> > > RuntimeExceptionInvoker(url);
> > >> >> > > Invocation invocation = new MockInvocation();
> > >> >> > > RpcStatus count = RpcStatus.getStatus(invoker.getUrl(),
> > >> >> > > invocation.getMethodName());
> > >> >> > > int beforeExceptionActiveCount = count.getActive();
> > >> >> > > try {
> > >> >> > > activeLimitFilter.invoke(invoker, invocation);
> > >> >> > > } catch (RuntimeException ex) {
> > >> >> > > int afterExceptionActiveCount = count.getActive();
> > >> >> > > assertEquals("After exception active count should be
> > same"
> > >> >> > > , beforeExceptionActiveCount,
> > >> >> afterExceptionActiveCount);
> > >> >> > > }
> > >> >> > > }
> > >> >> > >
> > >> >> > >
> > >> >> > > Where I am expecting RpcStatus active count before call and
> after
> > >> >> invoke
> > >> >> > > should be same, irrespective of exceptional handling by
> > >> >> ActiveLimitFilter
> > >> >> > > (e.g. in this case it should be 0). UT showing me that after
> > >> >> encountering
> > >> >> > > exception it is not same, on my further investigation I found
> > that
> > >> >> > >
> > >> >> > >
> > >> >> > > - If there is no *actives (ACTIVE_KEY) *is set or if its
> value
> > >> is
> > >> >> less
> > >> >> > > then *1* then it is always returning -1 (in UT active
> count),
> > >> which
> > >> >> > > means there is more number of call can be possible then it
> is
> > >> >> > > allowed(this
> > >> >> > > is my interpretation , correct me if this is wrong). e.g. if
> > >> >> within a
> > >> >> > > minute 10 call are allowed and if we encounter 5
> > >> *RuntimeException
> > >> >> > *then
> > >> >> > > in total we could landed up allowing 15 invoke. I am
> > suspecting
> > >> >> this
> > >> >> > is
> > >> >> > > because *max *being *0* we don't increment active count of
> > >> >> RpcStatus
> > >> >> > > and then decrease it in finally block where we have not even
> > >> have
> > >> >> > > increase
> > >> >> > > the count, because *max* is before the count increment
> > >> >> > >
> > >> >> > > if (max > 0 && !RpcStatus.beginCount(url,
> > >> methodName,
> > >> >> > max))
> > >> >> > >
> > >> >> > > As a reference I am copying the current master branch code of
> > >> >> > > ActiveLimitFilter.java. Would request to correct my
> understanding
> > >> if
> > >> >> it
> > >> >> > is
> > >> >> > > wrong, and if you feel my observation is correct then I would
> can
> > >> >> raise a
> > >> >> > > PR for fixing this issue.
> > >> >> > > *Note: UT is in my local I have not checked in into any branch.
> > >> >> > >
> > >> >> > > if (max > 0 && !RpcStatus.beginCount(url, methodName, max)) {
> > >> >> > > long timeout =
> > >> >> > > invoker.getUrl().getMethodParameter(invocation.getMethodName(),
> > >> >> > > Constants.TIMEOUT_KEY, 0);
> > >> >> > > long start = System.currentTimeMillis();
> > >> >> > > long remain = timeout;
> > >> >> > > synchronized (count) {
> > >> >> > > while (!RpcStatus.beginCount(url, methodName,
> > >> max)) {
> > >> >> > > try {
> > >> >> > > count.wait(remain);
> > >> >> > > } catch (InterruptedException e) {
> > >> >> > > // ignore
> > >> >> > > }
> > >> >> > > long elapsed = System.currentTimeMillis() -
> > >> start;
> > >> >> > > remain = timeout - elapsed;
> > >> >> > > if (remain <= 0) {
> > >> >> > > throw new RpcException("Waiting
> > concurrent
> > >> >> > > invoke timeout in client-side for service: "
> > >> >> > > +
> > invoker.getInterface().getName()
> > >> +
> > >> >> > > ", method: "
> > >> >> > > + invocation.getMethodName() +
> ",
> > >> >> > > elapsed: " + elapsed
> > >> >> > > + ", timeout: " + timeout + ".
> > >> >> > > concurrent invokes: " + count.getActive()
> > >> >> > > + ". max concurrent invoke
> limit:
> > >> " +
> > >> >> > max);
> > >> >> > > }
> > >> >> > > }
> > >> >> > > }
> > >> >> > > }
> > >> >> > >
> > >> >> > > boolean isSuccess = true;
> > >> >> > > long begin = System.currentTimeMillis();
> > >> >> > > try {
> > >> >> > > return invoker.invoke(invocation);
> > >> >> > > } catch (RuntimeException t) {
> > >> >> > > isSuccess = false;
> > >> >> > > throw t;
> > >> >> > > } finally {
> > >> >> > > RpcStatus.endCount(url, methodName,
> > >> >> > > System.currentTimeMillis() - begin, isSuccess);
> > >> >> > > if (max > 0) {
> > >> >> > > synchronized (count) {
> > >> >> > > count.notifyAll();
> > >> >> > > }
> > >> >> > > }
> > >> >> > > }
> > >> >> > >
> > >> >> >
> > >> >>
> > >> >
> > >>
> > >
> >
>
Re: ActiveLimitFilter some observation! Need your suggestion
Posted by Ian Luo <ia...@gmail.com>.
Imteyaz,
Would you mind to take a look at pull request 3035 [1] I submitted just now?
Thanks,
-Ian.
1. https://github.com/apache/incubator-dubbo/pull/3035
On Thu, Dec 20, 2018 at 4:41 PM Imteyaz Khan <kh...@gmail.com> wrote:
> Ian,
> Yes please, incorporate the fix. Will wait for your PR to review.
>
>
>
> On Thu, Dec 20, 2018 at 2:01 PM Imteyaz Khan <kh...@gmail.com>
> wrote:
>
> > Ian,
> > You won't believe, but what a coincidence I just loggined to gmail, to
> > reply on this tread to say, can I assist you on this issue any way and
> saw
> > your email. Any thing is fine with me.
> >
> > Regarding the apporach, as you mentioned endCount needs to be called I
> > certinly agree with you, but I was more looking for simplified approach
> of
> > changing if condition check where first check using beginCount then
> > followed by max>0. In this case, begin count will be always set and
> before
> > the end of method endCount will reduce it.
> >
> > Please correct me if I am wrong here.
> >
> > On Thu, Dec 20, 2018 at 1:54 PM Ian Luo <ia...@gmail.com> wrote:
> >
> >> Khan,
> >>
> >> I know you may busy on other stuff, should I jump on to give it a fix?
> >>
> >> Thanks,
> >> -Ian.
> >>
> >>
> >> On Thu, Dec 20, 2018 at 10:11 AM Ian Luo <ia...@gmail.com> wrote:
> >>
> >> > We cannot simply comment out 'endCount' in finally clause. If
> >> 'beginCount'
> >> > happens, then 'endCount' must be called.
> >> >
> >> > Thanks,
> >> > -Ian.
> >> >
> >> > On Wed, Dec 19, 2018 at 9:49 PM Imteyaz Khan <kh...@gmail.com>
> >> > wrote:
> >> >
> >> >> Ian,
> >> >> The approach you mentioned, I am certain it would works. I was
> >> thinking
> >> >> another approach
> >> >>
> >> >> if (!RpcStatus.beginCount(url, methodName, max) && max > 0) {
> >> >> }
> >> >>
> >> >> ...
> >> >> finally {
> >> >> //already existing endCount call.
> >> >> }
> >> >>
> >> >> Second approach I am mentioning because of simplification. Do let me
> >> know
> >> >> your thoughts on this.
> >> >>
> >> >>
> >> >> On Wed, Dec 19, 2018 at 6:39 PM Ian Luo <ia...@gmail.com> wrote:
> >> >>
> >> >> > Hi Khan,
> >> >> >
> >> >> > Thanks for pointing out this issue. I guess we could change logic
> to:
> >> >> >
> >> >> > if (max > 0 && !RpcStatus.beginCount(url, methodName, max)) {
> >> >> > ...
> >> >> > } else {
> >> >> > RpcStatus.beginCount(url, methodName);
> >> >> > }
> >> >> >
> >> >> > And we should do the same logic in ExecuteLimitFilter. Let me know
> >> your
> >> >> > opinion on this.
> >> >> >
> >> >> > Thanks,
> >> >> > -Ian.
> >> >> >
> >> >> >
> >> >> > On Wed, Dec 19, 2018 at 7:42 PM Imteyaz Khan <
> khan.imteyaz@gmail.com
> >> >
> >> >> > wrote:
> >> >> >
> >> >> > > Hi All,
> >> >> > > I was trying to write UT for ActiveLimitFilter for
> >> RuntimeException
> >> >> > > scenarion and below is my UT for the same
> >> >> > >
> >> >> > > ActiveLimitFilterTest.java
> >> >> > >
> >> >> > > @Test()
> >> >> > > public void testInvokeRuntimeExceptionWithActiveCountMatch() {
> >> >> > > URL url =
> >> >> > >
> >> >> >
> >> >>
> >>
> URL.valueOf("test://test:11/test?accesslog=true&group=dubbo&version=1.1&actives=0");
> >> >> > > Invoker<ActiveLimitFilterTest> invoker = new
> >> >> > > RuntimeExceptionInvoker(url);
> >> >> > > Invocation invocation = new MockInvocation();
> >> >> > > RpcStatus count = RpcStatus.getStatus(invoker.getUrl(),
> >> >> > > invocation.getMethodName());
> >> >> > > int beforeExceptionActiveCount = count.getActive();
> >> >> > > try {
> >> >> > > activeLimitFilter.invoke(invoker, invocation);
> >> >> > > } catch (RuntimeException ex) {
> >> >> > > int afterExceptionActiveCount = count.getActive();
> >> >> > > assertEquals("After exception active count should be
> same"
> >> >> > > , beforeExceptionActiveCount,
> >> >> afterExceptionActiveCount);
> >> >> > > }
> >> >> > > }
> >> >> > >
> >> >> > >
> >> >> > > Where I am expecting RpcStatus active count before call and after
> >> >> invoke
> >> >> > > should be same, irrespective of exceptional handling by
> >> >> ActiveLimitFilter
> >> >> > > (e.g. in this case it should be 0). UT showing me that after
> >> >> encountering
> >> >> > > exception it is not same, on my further investigation I found
> that
> >> >> > >
> >> >> > >
> >> >> > > - If there is no *actives (ACTIVE_KEY) *is set or if its value
> >> is
> >> >> less
> >> >> > > then *1* then it is always returning -1 (in UT active count),
> >> which
> >> >> > > means there is more number of call can be possible then it is
> >> >> > > allowed(this
> >> >> > > is my interpretation , correct me if this is wrong). e.g. if
> >> >> within a
> >> >> > > minute 10 call are allowed and if we encounter 5
> >> *RuntimeException
> >> >> > *then
> >> >> > > in total we could landed up allowing 15 invoke. I am
> suspecting
> >> >> this
> >> >> > is
> >> >> > > because *max *being *0* we don't increment active count of
> >> >> RpcStatus
> >> >> > > and then decrease it in finally block where we have not even
> >> have
> >> >> > > increase
> >> >> > > the count, because *max* is before the count increment
> >> >> > >
> >> >> > > if (max > 0 && !RpcStatus.beginCount(url,
> >> methodName,
> >> >> > max))
> >> >> > >
> >> >> > > As a reference I am copying the current master branch code of
> >> >> > > ActiveLimitFilter.java. Would request to correct my understanding
> >> if
> >> >> it
> >> >> > is
> >> >> > > wrong, and if you feel my observation is correct then I would can
> >> >> raise a
> >> >> > > PR for fixing this issue.
> >> >> > > *Note: UT is in my local I have not checked in into any branch.
> >> >> > >
> >> >> > > if (max > 0 && !RpcStatus.beginCount(url, methodName, max)) {
> >> >> > > long timeout =
> >> >> > > invoker.getUrl().getMethodParameter(invocation.getMethodName(),
> >> >> > > Constants.TIMEOUT_KEY, 0);
> >> >> > > long start = System.currentTimeMillis();
> >> >> > > long remain = timeout;
> >> >> > > synchronized (count) {
> >> >> > > while (!RpcStatus.beginCount(url, methodName,
> >> max)) {
> >> >> > > try {
> >> >> > > count.wait(remain);
> >> >> > > } catch (InterruptedException e) {
> >> >> > > // ignore
> >> >> > > }
> >> >> > > long elapsed = System.currentTimeMillis() -
> >> start;
> >> >> > > remain = timeout - elapsed;
> >> >> > > if (remain <= 0) {
> >> >> > > throw new RpcException("Waiting
> concurrent
> >> >> > > invoke timeout in client-side for service: "
> >> >> > > +
> invoker.getInterface().getName()
> >> +
> >> >> > > ", method: "
> >> >> > > + invocation.getMethodName() + ",
> >> >> > > elapsed: " + elapsed
> >> >> > > + ", timeout: " + timeout + ".
> >> >> > > concurrent invokes: " + count.getActive()
> >> >> > > + ". max concurrent invoke limit:
> >> " +
> >> >> > max);
> >> >> > > }
> >> >> > > }
> >> >> > > }
> >> >> > > }
> >> >> > >
> >> >> > > boolean isSuccess = true;
> >> >> > > long begin = System.currentTimeMillis();
> >> >> > > try {
> >> >> > > return invoker.invoke(invocation);
> >> >> > > } catch (RuntimeException t) {
> >> >> > > isSuccess = false;
> >> >> > > throw t;
> >> >> > > } finally {
> >> >> > > RpcStatus.endCount(url, methodName,
> >> >> > > System.currentTimeMillis() - begin, isSuccess);
> >> >> > > if (max > 0) {
> >> >> > > synchronized (count) {
> >> >> > > count.notifyAll();
> >> >> > > }
> >> >> > > }
> >> >> > > }
> >> >> > >
> >> >> >
> >> >>
> >> >
> >>
> >
>
Re: ActiveLimitFilter some observation! Need your suggestion
Posted by Imteyaz Khan <kh...@gmail.com>.
Ian,
Yes please, incorporate the fix. Will wait for your PR to review.
On Thu, Dec 20, 2018 at 2:01 PM Imteyaz Khan <kh...@gmail.com> wrote:
> Ian,
> You won't believe, but what a coincidence I just loggined to gmail, to
> reply on this tread to say, can I assist you on this issue any way and saw
> your email. Any thing is fine with me.
>
> Regarding the apporach, as you mentioned endCount needs to be called I
> certinly agree with you, but I was more looking for simplified approach of
> changing if condition check where first check using beginCount then
> followed by max>0. In this case, begin count will be always set and before
> the end of method endCount will reduce it.
>
> Please correct me if I am wrong here.
>
> On Thu, Dec 20, 2018 at 1:54 PM Ian Luo <ia...@gmail.com> wrote:
>
>> Khan,
>>
>> I know you may busy on other stuff, should I jump on to give it a fix?
>>
>> Thanks,
>> -Ian.
>>
>>
>> On Thu, Dec 20, 2018 at 10:11 AM Ian Luo <ia...@gmail.com> wrote:
>>
>> > We cannot simply comment out 'endCount' in finally clause. If
>> 'beginCount'
>> > happens, then 'endCount' must be called.
>> >
>> > Thanks,
>> > -Ian.
>> >
>> > On Wed, Dec 19, 2018 at 9:49 PM Imteyaz Khan <kh...@gmail.com>
>> > wrote:
>> >
>> >> Ian,
>> >> The approach you mentioned, I am certain it would works. I was
>> thinking
>> >> another approach
>> >>
>> >> if (!RpcStatus.beginCount(url, methodName, max) && max > 0) {
>> >> }
>> >>
>> >> ...
>> >> finally {
>> >> //already existing endCount call.
>> >> }
>> >>
>> >> Second approach I am mentioning because of simplification. Do let me
>> know
>> >> your thoughts on this.
>> >>
>> >>
>> >> On Wed, Dec 19, 2018 at 6:39 PM Ian Luo <ia...@gmail.com> wrote:
>> >>
>> >> > Hi Khan,
>> >> >
>> >> > Thanks for pointing out this issue. I guess we could change logic to:
>> >> >
>> >> > if (max > 0 && !RpcStatus.beginCount(url, methodName, max)) {
>> >> > ...
>> >> > } else {
>> >> > RpcStatus.beginCount(url, methodName);
>> >> > }
>> >> >
>> >> > And we should do the same logic in ExecuteLimitFilter. Let me know
>> your
>> >> > opinion on this.
>> >> >
>> >> > Thanks,
>> >> > -Ian.
>> >> >
>> >> >
>> >> > On Wed, Dec 19, 2018 at 7:42 PM Imteyaz Khan <khan.imteyaz@gmail.com
>> >
>> >> > wrote:
>> >> >
>> >> > > Hi All,
>> >> > > I was trying to write UT for ActiveLimitFilter for
>> RuntimeException
>> >> > > scenarion and below is my UT for the same
>> >> > >
>> >> > > ActiveLimitFilterTest.java
>> >> > >
>> >> > > @Test()
>> >> > > public void testInvokeRuntimeExceptionWithActiveCountMatch() {
>> >> > > URL url =
>> >> > >
>> >> >
>> >>
>> URL.valueOf("test://test:11/test?accesslog=true&group=dubbo&version=1.1&actives=0");
>> >> > > Invoker<ActiveLimitFilterTest> invoker = new
>> >> > > RuntimeExceptionInvoker(url);
>> >> > > Invocation invocation = new MockInvocation();
>> >> > > RpcStatus count = RpcStatus.getStatus(invoker.getUrl(),
>> >> > > invocation.getMethodName());
>> >> > > int beforeExceptionActiveCount = count.getActive();
>> >> > > try {
>> >> > > activeLimitFilter.invoke(invoker, invocation);
>> >> > > } catch (RuntimeException ex) {
>> >> > > int afterExceptionActiveCount = count.getActive();
>> >> > > assertEquals("After exception active count should be same"
>> >> > > , beforeExceptionActiveCount,
>> >> afterExceptionActiveCount);
>> >> > > }
>> >> > > }
>> >> > >
>> >> > >
>> >> > > Where I am expecting RpcStatus active count before call and after
>> >> invoke
>> >> > > should be same, irrespective of exceptional handling by
>> >> ActiveLimitFilter
>> >> > > (e.g. in this case it should be 0). UT showing me that after
>> >> encountering
>> >> > > exception it is not same, on my further investigation I found that
>> >> > >
>> >> > >
>> >> > > - If there is no *actives (ACTIVE_KEY) *is set or if its value
>> is
>> >> less
>> >> > > then *1* then it is always returning -1 (in UT active count),
>> which
>> >> > > means there is more number of call can be possible then it is
>> >> > > allowed(this
>> >> > > is my interpretation , correct me if this is wrong). e.g. if
>> >> within a
>> >> > > minute 10 call are allowed and if we encounter 5
>> *RuntimeException
>> >> > *then
>> >> > > in total we could landed up allowing 15 invoke. I am suspecting
>> >> this
>> >> > is
>> >> > > because *max *being *0* we don't increment active count of
>> >> RpcStatus
>> >> > > and then decrease it in finally block where we have not even
>> have
>> >> > > increase
>> >> > > the count, because *max* is before the count increment
>> >> > >
>> >> > > if (max > 0 && !RpcStatus.beginCount(url,
>> methodName,
>> >> > max))
>> >> > >
>> >> > > As a reference I am copying the current master branch code of
>> >> > > ActiveLimitFilter.java. Would request to correct my understanding
>> if
>> >> it
>> >> > is
>> >> > > wrong, and if you feel my observation is correct then I would can
>> >> raise a
>> >> > > PR for fixing this issue.
>> >> > > *Note: UT is in my local I have not checked in into any branch.
>> >> > >
>> >> > > if (max > 0 && !RpcStatus.beginCount(url, methodName, max)) {
>> >> > > long timeout =
>> >> > > invoker.getUrl().getMethodParameter(invocation.getMethodName(),
>> >> > > Constants.TIMEOUT_KEY, 0);
>> >> > > long start = System.currentTimeMillis();
>> >> > > long remain = timeout;
>> >> > > synchronized (count) {
>> >> > > while (!RpcStatus.beginCount(url, methodName,
>> max)) {
>> >> > > try {
>> >> > > count.wait(remain);
>> >> > > } catch (InterruptedException e) {
>> >> > > // ignore
>> >> > > }
>> >> > > long elapsed = System.currentTimeMillis() -
>> start;
>> >> > > remain = timeout - elapsed;
>> >> > > if (remain <= 0) {
>> >> > > throw new RpcException("Waiting concurrent
>> >> > > invoke timeout in client-side for service: "
>> >> > > + invoker.getInterface().getName()
>> +
>> >> > > ", method: "
>> >> > > + invocation.getMethodName() + ",
>> >> > > elapsed: " + elapsed
>> >> > > + ", timeout: " + timeout + ".
>> >> > > concurrent invokes: " + count.getActive()
>> >> > > + ". max concurrent invoke limit:
>> " +
>> >> > max);
>> >> > > }
>> >> > > }
>> >> > > }
>> >> > > }
>> >> > >
>> >> > > boolean isSuccess = true;
>> >> > > long begin = System.currentTimeMillis();
>> >> > > try {
>> >> > > return invoker.invoke(invocation);
>> >> > > } catch (RuntimeException t) {
>> >> > > isSuccess = false;
>> >> > > throw t;
>> >> > > } finally {
>> >> > > RpcStatus.endCount(url, methodName,
>> >> > > System.currentTimeMillis() - begin, isSuccess);
>> >> > > if (max > 0) {
>> >> > > synchronized (count) {
>> >> > > count.notifyAll();
>> >> > > }
>> >> > > }
>> >> > > }
>> >> > >
>> >> >
>> >>
>> >
>>
>
Re: ActiveLimitFilter some observation! Need your suggestion
Posted by Imteyaz Khan <kh...@gmail.com>.
Ian,
You won't believe, but what a coincidence I just loggined to gmail, to
reply on this tread to say, can I assist you on this issue any way and saw
your email. Any thing is fine with me.
Regarding the apporach, as you mentioned endCount needs to be called I
certinly agree with you, but I was more looking for simplified approach of
changing if condition check where first check using beginCount then
followed by max>0. In this case, begin count will be always set and before
the end of method endCount will reduce it.
Please correct me if I am wrong here.
On Thu, Dec 20, 2018 at 1:54 PM Ian Luo <ia...@gmail.com> wrote:
> Khan,
>
> I know you may busy on other stuff, should I jump on to give it a fix?
>
> Thanks,
> -Ian.
>
>
> On Thu, Dec 20, 2018 at 10:11 AM Ian Luo <ia...@gmail.com> wrote:
>
> > We cannot simply comment out 'endCount' in finally clause. If
> 'beginCount'
> > happens, then 'endCount' must be called.
> >
> > Thanks,
> > -Ian.
> >
> > On Wed, Dec 19, 2018 at 9:49 PM Imteyaz Khan <kh...@gmail.com>
> > wrote:
> >
> >> Ian,
> >> The approach you mentioned, I am certain it would works. I was
> thinking
> >> another approach
> >>
> >> if (!RpcStatus.beginCount(url, methodName, max) && max > 0) {
> >> }
> >>
> >> ...
> >> finally {
> >> //already existing endCount call.
> >> }
> >>
> >> Second approach I am mentioning because of simplification. Do let me
> know
> >> your thoughts on this.
> >>
> >>
> >> On Wed, Dec 19, 2018 at 6:39 PM Ian Luo <ia...@gmail.com> wrote:
> >>
> >> > Hi Khan,
> >> >
> >> > Thanks for pointing out this issue. I guess we could change logic to:
> >> >
> >> > if (max > 0 && !RpcStatus.beginCount(url, methodName, max)) {
> >> > ...
> >> > } else {
> >> > RpcStatus.beginCount(url, methodName);
> >> > }
> >> >
> >> > And we should do the same logic in ExecuteLimitFilter. Let me know
> your
> >> > opinion on this.
> >> >
> >> > Thanks,
> >> > -Ian.
> >> >
> >> >
> >> > On Wed, Dec 19, 2018 at 7:42 PM Imteyaz Khan <kh...@gmail.com>
> >> > wrote:
> >> >
> >> > > Hi All,
> >> > > I was trying to write UT for ActiveLimitFilter for
> RuntimeException
> >> > > scenarion and below is my UT for the same
> >> > >
> >> > > ActiveLimitFilterTest.java
> >> > >
> >> > > @Test()
> >> > > public void testInvokeRuntimeExceptionWithActiveCountMatch() {
> >> > > URL url =
> >> > >
> >> >
> >>
> URL.valueOf("test://test:11/test?accesslog=true&group=dubbo&version=1.1&actives=0");
> >> > > Invoker<ActiveLimitFilterTest> invoker = new
> >> > > RuntimeExceptionInvoker(url);
> >> > > Invocation invocation = new MockInvocation();
> >> > > RpcStatus count = RpcStatus.getStatus(invoker.getUrl(),
> >> > > invocation.getMethodName());
> >> > > int beforeExceptionActiveCount = count.getActive();
> >> > > try {
> >> > > activeLimitFilter.invoke(invoker, invocation);
> >> > > } catch (RuntimeException ex) {
> >> > > int afterExceptionActiveCount = count.getActive();
> >> > > assertEquals("After exception active count should be same"
> >> > > , beforeExceptionActiveCount,
> >> afterExceptionActiveCount);
> >> > > }
> >> > > }
> >> > >
> >> > >
> >> > > Where I am expecting RpcStatus active count before call and after
> >> invoke
> >> > > should be same, irrespective of exceptional handling by
> >> ActiveLimitFilter
> >> > > (e.g. in this case it should be 0). UT showing me that after
> >> encountering
> >> > > exception it is not same, on my further investigation I found that
> >> > >
> >> > >
> >> > > - If there is no *actives (ACTIVE_KEY) *is set or if its value is
> >> less
> >> > > then *1* then it is always returning -1 (in UT active count),
> which
> >> > > means there is more number of call can be possible then it is
> >> > > allowed(this
> >> > > is my interpretation , correct me if this is wrong). e.g. if
> >> within a
> >> > > minute 10 call are allowed and if we encounter 5
> *RuntimeException
> >> > *then
> >> > > in total we could landed up allowing 15 invoke. I am suspecting
> >> this
> >> > is
> >> > > because *max *being *0* we don't increment active count of
> >> RpcStatus
> >> > > and then decrease it in finally block where we have not even have
> >> > > increase
> >> > > the count, because *max* is before the count increment
> >> > >
> >> > > if (max > 0 && !RpcStatus.beginCount(url,
> methodName,
> >> > max))
> >> > >
> >> > > As a reference I am copying the current master branch code of
> >> > > ActiveLimitFilter.java. Would request to correct my understanding if
> >> it
> >> > is
> >> > > wrong, and if you feel my observation is correct then I would can
> >> raise a
> >> > > PR for fixing this issue.
> >> > > *Note: UT is in my local I have not checked in into any branch.
> >> > >
> >> > > if (max > 0 && !RpcStatus.beginCount(url, methodName, max)) {
> >> > > long timeout =
> >> > > invoker.getUrl().getMethodParameter(invocation.getMethodName(),
> >> > > Constants.TIMEOUT_KEY, 0);
> >> > > long start = System.currentTimeMillis();
> >> > > long remain = timeout;
> >> > > synchronized (count) {
> >> > > while (!RpcStatus.beginCount(url, methodName, max))
> {
> >> > > try {
> >> > > count.wait(remain);
> >> > > } catch (InterruptedException e) {
> >> > > // ignore
> >> > > }
> >> > > long elapsed = System.currentTimeMillis() -
> start;
> >> > > remain = timeout - elapsed;
> >> > > if (remain <= 0) {
> >> > > throw new RpcException("Waiting concurrent
> >> > > invoke timeout in client-side for service: "
> >> > > + invoker.getInterface().getName() +
> >> > > ", method: "
> >> > > + invocation.getMethodName() + ",
> >> > > elapsed: " + elapsed
> >> > > + ", timeout: " + timeout + ".
> >> > > concurrent invokes: " + count.getActive()
> >> > > + ". max concurrent invoke limit: "
> +
> >> > max);
> >> > > }
> >> > > }
> >> > > }
> >> > > }
> >> > >
> >> > > boolean isSuccess = true;
> >> > > long begin = System.currentTimeMillis();
> >> > > try {
> >> > > return invoker.invoke(invocation);
> >> > > } catch (RuntimeException t) {
> >> > > isSuccess = false;
> >> > > throw t;
> >> > > } finally {
> >> > > RpcStatus.endCount(url, methodName,
> >> > > System.currentTimeMillis() - begin, isSuccess);
> >> > > if (max > 0) {
> >> > > synchronized (count) {
> >> > > count.notifyAll();
> >> > > }
> >> > > }
> >> > > }
> >> > >
> >> >
> >>
> >
>
Re: ActiveLimitFilter some observation! Need your suggestion
Posted by Ian Luo <ia...@gmail.com>.
Khan,
I know you may busy on other stuff, should I jump on to give it a fix?
Thanks,
-Ian.
On Thu, Dec 20, 2018 at 10:11 AM Ian Luo <ia...@gmail.com> wrote:
> We cannot simply comment out 'endCount' in finally clause. If 'beginCount'
> happens, then 'endCount' must be called.
>
> Thanks,
> -Ian.
>
> On Wed, Dec 19, 2018 at 9:49 PM Imteyaz Khan <kh...@gmail.com>
> wrote:
>
>> Ian,
>> The approach you mentioned, I am certain it would works. I was thinking
>> another approach
>>
>> if (!RpcStatus.beginCount(url, methodName, max) && max > 0) {
>> }
>>
>> ...
>> finally {
>> //already existing endCount call.
>> }
>>
>> Second approach I am mentioning because of simplification. Do let me know
>> your thoughts on this.
>>
>>
>> On Wed, Dec 19, 2018 at 6:39 PM Ian Luo <ia...@gmail.com> wrote:
>>
>> > Hi Khan,
>> >
>> > Thanks for pointing out this issue. I guess we could change logic to:
>> >
>> > if (max > 0 && !RpcStatus.beginCount(url, methodName, max)) {
>> > ...
>> > } else {
>> > RpcStatus.beginCount(url, methodName);
>> > }
>> >
>> > And we should do the same logic in ExecuteLimitFilter. Let me know your
>> > opinion on this.
>> >
>> > Thanks,
>> > -Ian.
>> >
>> >
>> > On Wed, Dec 19, 2018 at 7:42 PM Imteyaz Khan <kh...@gmail.com>
>> > wrote:
>> >
>> > > Hi All,
>> > > I was trying to write UT for ActiveLimitFilter for RuntimeException
>> > > scenarion and below is my UT for the same
>> > >
>> > > ActiveLimitFilterTest.java
>> > >
>> > > @Test()
>> > > public void testInvokeRuntimeExceptionWithActiveCountMatch() {
>> > > URL url =
>> > >
>> >
>> URL.valueOf("test://test:11/test?accesslog=true&group=dubbo&version=1.1&actives=0");
>> > > Invoker<ActiveLimitFilterTest> invoker = new
>> > > RuntimeExceptionInvoker(url);
>> > > Invocation invocation = new MockInvocation();
>> > > RpcStatus count = RpcStatus.getStatus(invoker.getUrl(),
>> > > invocation.getMethodName());
>> > > int beforeExceptionActiveCount = count.getActive();
>> > > try {
>> > > activeLimitFilter.invoke(invoker, invocation);
>> > > } catch (RuntimeException ex) {
>> > > int afterExceptionActiveCount = count.getActive();
>> > > assertEquals("After exception active count should be same"
>> > > , beforeExceptionActiveCount,
>> afterExceptionActiveCount);
>> > > }
>> > > }
>> > >
>> > >
>> > > Where I am expecting RpcStatus active count before call and after
>> invoke
>> > > should be same, irrespective of exceptional handling by
>> ActiveLimitFilter
>> > > (e.g. in this case it should be 0). UT showing me that after
>> encountering
>> > > exception it is not same, on my further investigation I found that
>> > >
>> > >
>> > > - If there is no *actives (ACTIVE_KEY) *is set or if its value is
>> less
>> > > then *1* then it is always returning -1 (in UT active count), which
>> > > means there is more number of call can be possible then it is
>> > > allowed(this
>> > > is my interpretation , correct me if this is wrong). e.g. if
>> within a
>> > > minute 10 call are allowed and if we encounter 5 *RuntimeException
>> > *then
>> > > in total we could landed up allowing 15 invoke. I am suspecting
>> this
>> > is
>> > > because *max *being *0* we don't increment active count of
>> RpcStatus
>> > > and then decrease it in finally block where we have not even have
>> > > increase
>> > > the count, because *max* is before the count increment
>> > >
>> > > if (max > 0 && !RpcStatus.beginCount(url, methodName,
>> > max))
>> > >
>> > > As a reference I am copying the current master branch code of
>> > > ActiveLimitFilter.java. Would request to correct my understanding if
>> it
>> > is
>> > > wrong, and if you feel my observation is correct then I would can
>> raise a
>> > > PR for fixing this issue.
>> > > *Note: UT is in my local I have not checked in into any branch.
>> > >
>> > > if (max > 0 && !RpcStatus.beginCount(url, methodName, max)) {
>> > > long timeout =
>> > > invoker.getUrl().getMethodParameter(invocation.getMethodName(),
>> > > Constants.TIMEOUT_KEY, 0);
>> > > long start = System.currentTimeMillis();
>> > > long remain = timeout;
>> > > synchronized (count) {
>> > > while (!RpcStatus.beginCount(url, methodName, max)) {
>> > > try {
>> > > count.wait(remain);
>> > > } catch (InterruptedException e) {
>> > > // ignore
>> > > }
>> > > long elapsed = System.currentTimeMillis() - start;
>> > > remain = timeout - elapsed;
>> > > if (remain <= 0) {
>> > > throw new RpcException("Waiting concurrent
>> > > invoke timeout in client-side for service: "
>> > > + invoker.getInterface().getName() +
>> > > ", method: "
>> > > + invocation.getMethodName() + ",
>> > > elapsed: " + elapsed
>> > > + ", timeout: " + timeout + ".
>> > > concurrent invokes: " + count.getActive()
>> > > + ". max concurrent invoke limit: " +
>> > max);
>> > > }
>> > > }
>> > > }
>> > > }
>> > >
>> > > boolean isSuccess = true;
>> > > long begin = System.currentTimeMillis();
>> > > try {
>> > > return invoker.invoke(invocation);
>> > > } catch (RuntimeException t) {
>> > > isSuccess = false;
>> > > throw t;
>> > > } finally {
>> > > RpcStatus.endCount(url, methodName,
>> > > System.currentTimeMillis() - begin, isSuccess);
>> > > if (max > 0) {
>> > > synchronized (count) {
>> > > count.notifyAll();
>> > > }
>> > > }
>> > > }
>> > >
>> >
>>
>
Re: ActiveLimitFilter some observation! Need your suggestion
Posted by Ian Luo <ia...@gmail.com>.
We cannot simply comment out 'endCount' in finally clause. If 'beginCount'
happens, then 'endCount' must be called.
Thanks,
-Ian.
On Wed, Dec 19, 2018 at 9:49 PM Imteyaz Khan <kh...@gmail.com> wrote:
> Ian,
> The approach you mentioned, I am certain it would works. I was thinking
> another approach
>
> if (!RpcStatus.beginCount(url, methodName, max) && max > 0) {
> }
>
> ...
> finally {
> //already existing endCount call.
> }
>
> Second approach I am mentioning because of simplification. Do let me know
> your thoughts on this.
>
>
> On Wed, Dec 19, 2018 at 6:39 PM Ian Luo <ia...@gmail.com> wrote:
>
> > Hi Khan,
> >
> > Thanks for pointing out this issue. I guess we could change logic to:
> >
> > if (max > 0 && !RpcStatus.beginCount(url, methodName, max)) {
> > ...
> > } else {
> > RpcStatus.beginCount(url, methodName);
> > }
> >
> > And we should do the same logic in ExecuteLimitFilter. Let me know your
> > opinion on this.
> >
> > Thanks,
> > -Ian.
> >
> >
> > On Wed, Dec 19, 2018 at 7:42 PM Imteyaz Khan <kh...@gmail.com>
> > wrote:
> >
> > > Hi All,
> > > I was trying to write UT for ActiveLimitFilter for RuntimeException
> > > scenarion and below is my UT for the same
> > >
> > > ActiveLimitFilterTest.java
> > >
> > > @Test()
> > > public void testInvokeRuntimeExceptionWithActiveCountMatch() {
> > > URL url =
> > >
> >
> URL.valueOf("test://test:11/test?accesslog=true&group=dubbo&version=1.1&actives=0");
> > > Invoker<ActiveLimitFilterTest> invoker = new
> > > RuntimeExceptionInvoker(url);
> > > Invocation invocation = new MockInvocation();
> > > RpcStatus count = RpcStatus.getStatus(invoker.getUrl(),
> > > invocation.getMethodName());
> > > int beforeExceptionActiveCount = count.getActive();
> > > try {
> > > activeLimitFilter.invoke(invoker, invocation);
> > > } catch (RuntimeException ex) {
> > > int afterExceptionActiveCount = count.getActive();
> > > assertEquals("After exception active count should be same"
> > > , beforeExceptionActiveCount,
> afterExceptionActiveCount);
> > > }
> > > }
> > >
> > >
> > > Where I am expecting RpcStatus active count before call and after
> invoke
> > > should be same, irrespective of exceptional handling by
> ActiveLimitFilter
> > > (e.g. in this case it should be 0). UT showing me that after
> encountering
> > > exception it is not same, on my further investigation I found that
> > >
> > >
> > > - If there is no *actives (ACTIVE_KEY) *is set or if its value is
> less
> > > then *1* then it is always returning -1 (in UT active count), which
> > > means there is more number of call can be possible then it is
> > > allowed(this
> > > is my interpretation , correct me if this is wrong). e.g. if within
> a
> > > minute 10 call are allowed and if we encounter 5 *RuntimeException
> > *then
> > > in total we could landed up allowing 15 invoke. I am suspecting this
> > is
> > > because *max *being *0* we don't increment active count of
> RpcStatus
> > > and then decrease it in finally block where we have not even have
> > > increase
> > > the count, because *max* is before the count increment
> > >
> > > if (max > 0 && !RpcStatus.beginCount(url, methodName,
> > max))
> > >
> > > As a reference I am copying the current master branch code of
> > > ActiveLimitFilter.java. Would request to correct my understanding if it
> > is
> > > wrong, and if you feel my observation is correct then I would can
> raise a
> > > PR for fixing this issue.
> > > *Note: UT is in my local I have not checked in into any branch.
> > >
> > > if (max > 0 && !RpcStatus.beginCount(url, methodName, max)) {
> > > long timeout =
> > > invoker.getUrl().getMethodParameter(invocation.getMethodName(),
> > > Constants.TIMEOUT_KEY, 0);
> > > long start = System.currentTimeMillis();
> > > long remain = timeout;
> > > synchronized (count) {
> > > while (!RpcStatus.beginCount(url, methodName, max)) {
> > > try {
> > > count.wait(remain);
> > > } catch (InterruptedException e) {
> > > // ignore
> > > }
> > > long elapsed = System.currentTimeMillis() - start;
> > > remain = timeout - elapsed;
> > > if (remain <= 0) {
> > > throw new RpcException("Waiting concurrent
> > > invoke timeout in client-side for service: "
> > > + invoker.getInterface().getName() +
> > > ", method: "
> > > + invocation.getMethodName() + ",
> > > elapsed: " + elapsed
> > > + ", timeout: " + timeout + ".
> > > concurrent invokes: " + count.getActive()
> > > + ". max concurrent invoke limit: " +
> > max);
> > > }
> > > }
> > > }
> > > }
> > >
> > > boolean isSuccess = true;
> > > long begin = System.currentTimeMillis();
> > > try {
> > > return invoker.invoke(invocation);
> > > } catch (RuntimeException t) {
> > > isSuccess = false;
> > > throw t;
> > > } finally {
> > > RpcStatus.endCount(url, methodName,
> > > System.currentTimeMillis() - begin, isSuccess);
> > > if (max > 0) {
> > > synchronized (count) {
> > > count.notifyAll();
> > > }
> > > }
> > > }
> > >
> >
>
Re: ActiveLimitFilter some observation! Need your suggestion
Posted by Imteyaz Khan <kh...@gmail.com>.
Ian,
The approach you mentioned, I am certain it would works. I was thinking
another approach
if (!RpcStatus.beginCount(url, methodName, max) && max > 0) {
}
...
finally {
//already existing endCount call.
}
Second approach I am mentioning because of simplification. Do let me know
your thoughts on this.
On Wed, Dec 19, 2018 at 6:39 PM Ian Luo <ia...@gmail.com> wrote:
> Hi Khan,
>
> Thanks for pointing out this issue. I guess we could change logic to:
>
> if (max > 0 && !RpcStatus.beginCount(url, methodName, max)) {
> ...
> } else {
> RpcStatus.beginCount(url, methodName);
> }
>
> And we should do the same logic in ExecuteLimitFilter. Let me know your
> opinion on this.
>
> Thanks,
> -Ian.
>
>
> On Wed, Dec 19, 2018 at 7:42 PM Imteyaz Khan <kh...@gmail.com>
> wrote:
>
> > Hi All,
> > I was trying to write UT for ActiveLimitFilter for RuntimeException
> > scenarion and below is my UT for the same
> >
> > ActiveLimitFilterTest.java
> >
> > @Test()
> > public void testInvokeRuntimeExceptionWithActiveCountMatch() {
> > URL url =
> >
> URL.valueOf("test://test:11/test?accesslog=true&group=dubbo&version=1.1&actives=0");
> > Invoker<ActiveLimitFilterTest> invoker = new
> > RuntimeExceptionInvoker(url);
> > Invocation invocation = new MockInvocation();
> > RpcStatus count = RpcStatus.getStatus(invoker.getUrl(),
> > invocation.getMethodName());
> > int beforeExceptionActiveCount = count.getActive();
> > try {
> > activeLimitFilter.invoke(invoker, invocation);
> > } catch (RuntimeException ex) {
> > int afterExceptionActiveCount = count.getActive();
> > assertEquals("After exception active count should be same"
> > , beforeExceptionActiveCount, afterExceptionActiveCount);
> > }
> > }
> >
> >
> > Where I am expecting RpcStatus active count before call and after invoke
> > should be same, irrespective of exceptional handling by ActiveLimitFilter
> > (e.g. in this case it should be 0). UT showing me that after encountering
> > exception it is not same, on my further investigation I found that
> >
> >
> > - If there is no *actives (ACTIVE_KEY) *is set or if its value is less
> > then *1* then it is always returning -1 (in UT active count), which
> > means there is more number of call can be possible then it is
> > allowed(this
> > is my interpretation , correct me if this is wrong). e.g. if within a
> > minute 10 call are allowed and if we encounter 5 *RuntimeException
> *then
> > in total we could landed up allowing 15 invoke. I am suspecting this
> is
> > because *max *being *0* we don't increment active count of RpcStatus
> > and then decrease it in finally block where we have not even have
> > increase
> > the count, because *max* is before the count increment
> >
> > if (max > 0 && !RpcStatus.beginCount(url, methodName,
> max))
> >
> > As a reference I am copying the current master branch code of
> > ActiveLimitFilter.java. Would request to correct my understanding if it
> is
> > wrong, and if you feel my observation is correct then I would can raise a
> > PR for fixing this issue.
> > *Note: UT is in my local I have not checked in into any branch.
> >
> > if (max > 0 && !RpcStatus.beginCount(url, methodName, max)) {
> > long timeout =
> > invoker.getUrl().getMethodParameter(invocation.getMethodName(),
> > Constants.TIMEOUT_KEY, 0);
> > long start = System.currentTimeMillis();
> > long remain = timeout;
> > synchronized (count) {
> > while (!RpcStatus.beginCount(url, methodName, max)) {
> > try {
> > count.wait(remain);
> > } catch (InterruptedException e) {
> > // ignore
> > }
> > long elapsed = System.currentTimeMillis() - start;
> > remain = timeout - elapsed;
> > if (remain <= 0) {
> > throw new RpcException("Waiting concurrent
> > invoke timeout in client-side for service: "
> > + invoker.getInterface().getName() +
> > ", method: "
> > + invocation.getMethodName() + ",
> > elapsed: " + elapsed
> > + ", timeout: " + timeout + ".
> > concurrent invokes: " + count.getActive()
> > + ". max concurrent invoke limit: " +
> max);
> > }
> > }
> > }
> > }
> >
> > boolean isSuccess = true;
> > long begin = System.currentTimeMillis();
> > try {
> > return invoker.invoke(invocation);
> > } catch (RuntimeException t) {
> > isSuccess = false;
> > throw t;
> > } finally {
> > RpcStatus.endCount(url, methodName,
> > System.currentTimeMillis() - begin, isSuccess);
> > if (max > 0) {
> > synchronized (count) {
> > count.notifyAll();
> > }
> > }
> > }
> >
>
Re: ActiveLimitFilter some observation! Need your suggestion
Posted by Ian Luo <ia...@gmail.com>.
Hi Khan,
Thanks for pointing out this issue. I guess we could change logic to:
if (max > 0 && !RpcStatus.beginCount(url, methodName, max)) {
...
} else {
RpcStatus.beginCount(url, methodName);
}
And we should do the same logic in ExecuteLimitFilter. Let me know your
opinion on this.
Thanks,
-Ian.
On Wed, Dec 19, 2018 at 7:42 PM Imteyaz Khan <kh...@gmail.com> wrote:
> Hi All,
> I was trying to write UT for ActiveLimitFilter for RuntimeException
> scenarion and below is my UT for the same
>
> ActiveLimitFilterTest.java
>
> @Test()
> public void testInvokeRuntimeExceptionWithActiveCountMatch() {
> URL url =
> URL.valueOf("test://test:11/test?accesslog=true&group=dubbo&version=1.1&actives=0");
> Invoker<ActiveLimitFilterTest> invoker = new
> RuntimeExceptionInvoker(url);
> Invocation invocation = new MockInvocation();
> RpcStatus count = RpcStatus.getStatus(invoker.getUrl(),
> invocation.getMethodName());
> int beforeExceptionActiveCount = count.getActive();
> try {
> activeLimitFilter.invoke(invoker, invocation);
> } catch (RuntimeException ex) {
> int afterExceptionActiveCount = count.getActive();
> assertEquals("After exception active count should be same"
> , beforeExceptionActiveCount, afterExceptionActiveCount);
> }
> }
>
>
> Where I am expecting RpcStatus active count before call and after invoke
> should be same, irrespective of exceptional handling by ActiveLimitFilter
> (e.g. in this case it should be 0). UT showing me that after encountering
> exception it is not same, on my further investigation I found that
>
>
> - If there is no *actives (ACTIVE_KEY) *is set or if its value is less
> then *1* then it is always returning -1 (in UT active count), which
> means there is more number of call can be possible then it is
> allowed(this
> is my interpretation , correct me if this is wrong). e.g. if within a
> minute 10 call are allowed and if we encounter 5 *RuntimeException *then
> in total we could landed up allowing 15 invoke. I am suspecting this is
> because *max *being *0* we don't increment active count of RpcStatus
> and then decrease it in finally block where we have not even have
> increase
> the count, because *max* is before the count increment
>
> if (max > 0 && !RpcStatus.beginCount(url, methodName, max))
>
> As a reference I am copying the current master branch code of
> ActiveLimitFilter.java. Would request to correct my understanding if it is
> wrong, and if you feel my observation is correct then I would can raise a
> PR for fixing this issue.
> *Note: UT is in my local I have not checked in into any branch.
>
> if (max > 0 && !RpcStatus.beginCount(url, methodName, max)) {
> long timeout =
> invoker.getUrl().getMethodParameter(invocation.getMethodName(),
> Constants.TIMEOUT_KEY, 0);
> long start = System.currentTimeMillis();
> long remain = timeout;
> synchronized (count) {
> while (!RpcStatus.beginCount(url, methodName, max)) {
> try {
> count.wait(remain);
> } catch (InterruptedException e) {
> // ignore
> }
> long elapsed = System.currentTimeMillis() - start;
> remain = timeout - elapsed;
> if (remain <= 0) {
> throw new RpcException("Waiting concurrent
> invoke timeout in client-side for service: "
> + invoker.getInterface().getName() +
> ", method: "
> + invocation.getMethodName() + ",
> elapsed: " + elapsed
> + ", timeout: " + timeout + ".
> concurrent invokes: " + count.getActive()
> + ". max concurrent invoke limit: " + max);
> }
> }
> }
> }
>
> boolean isSuccess = true;
> long begin = System.currentTimeMillis();
> try {
> return invoker.invoke(invocation);
> } catch (RuntimeException t) {
> isSuccess = false;
> throw t;
> } finally {
> RpcStatus.endCount(url, methodName,
> System.currentTimeMillis() - begin, isSuccess);
> if (max > 0) {
> synchronized (count) {
> count.notifyAll();
> }
> }
> }
>