Ian, Sure, on it. On Fri, Dec 21, 2018 at 8:34 AM Ian Luo <[email protected]> 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 <[email protected]> > 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 <[email protected]> > > 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 <[email protected]> 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 <[email protected]> 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 < > [email protected]> > > >> > 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 <[email protected]> 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 < > > [email protected] > > >> > > > >> >> > 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(); > > >> >> > > } > > >> >> > > } > > >> >> > > } > > >> >> > > > > >> >> > > > >> >> > > >> > > > >> > > > > > >
