Re: [Dev] Debug statements in TenantAwareLoadBalanceEndpoint
Nirmal,
We should also check the dispatch component in synapse[1], specially
AbstractDispatcher#removeSessionID, where it has debug level
logs without the correct wrap. I think this was added for a recent ELB
related fix.
Thanks,
Kishanthan.
[1]
https://svn.wso2.org/repos/wso2/carbon/platform/trunk/dependencies/synapse/2.1.1-wso2v8/modules/core/src/main/java/org/apache/synapse/endpoints/dispatch/
On Fri, Aug 16, 2013 at 8:54 AM, Nirmal Fernando nir...@wso2.com wrote:
Hi Azeez,
I grepped the class:
nirmal@nirmal:/media/wso2/carbon/platform/trunk/components/load-balancer/lb-endpoint/org.wso2.carbon.lb.endpoint$
grep -nr log.isDebug
./src/main/java/org/wso2/carbon/lb/endpoint/endpoint/TenantAwareLoadBalanceEndpoint.java
142:if (log.isDebugEnabled()) {
268:if (log.isDebugEnabled()) {
nirmal@nirmal:/media/wso2/carbon/platform/trunk/components/load-balancer/lb-endpoint/org.wso2.carbon.lb.endpoint$
nirmal@nirmal:/media/wso2/carbon/platform/trunk/components/load-balancer/lb-endpoint/org.wso2.carbon.lb.endpoint$
nirmal@nirmal:/media/wso2/carbon/platform/trunk/components/load-balancer/lb-endpoint/org.wso2.carbon.lb.endpoint$
nirmal@nirmal:/media/wso2/carbon/platform/trunk/components/load-balancer/lb-endpoint/org.wso2.carbon.lb.endpoint$
nirmal@nirmal:/media/wso2/carbon/platform/trunk/components/load-balancer/lb-endpoint/org.wso2.carbon.lb.endpoint$
nirmal@nirmal:/media/wso2/carbon/platform/trunk/components/load-balancer/lb-endpoint/org.wso2.carbon.lb.endpoint$
grep -nr log.debug
./src/main/java/org/wso2/carbon/lb/endpoint/endpoint/TenantAwareLoadBalanceEndpoint.java
143:log.debug(Group management agent
added to cluster domain: +
269:log.debug(Current session id : +
sessionInformation.getId());
299:log.debug(* Actual Host: +actualHost + **
Target Host: +targetHost);
328:log.debug(* Actual Host: +actualHost
+ ** Target Host: +targetHost);
431:log.debug(Setting host name: +host);
454:log.debug(Retrying to a failed member has stopped.);
nirmal@nirmal
:/media/wso2/carbon/platform/trunk/components/load-balancer/lb-endpoint/org.wso2.carbon.lb.endpoint$
Those in green are wrapped using log.isDebugEnabled.
One in beige has no concatenation.
I guess the concern is on ones in red. +1 for wrapping those.
On Fri, Aug 16, 2013 at 4:43 PM, Afkham Azeez az...@wso2.com wrote:
There are a number of debug statements which do String concatenations,
and are not wrapped in log.isDebugEnabled() conditions. Such statements
will cause unnecessary performance overhead.
--
*Afkham Azeez*
Director of Architecture; WSO2, Inc.; http://wso2.com
Member; Apache Software Foundation; http://www.apache.org/
* http://www.apache.org/**
email: **az...@wso2.com* az...@wso2.com* cell: +94 77 3320919
blog: **http://blog.afkham.org* http://blog.afkham.org*
twitter: **http://twitter.com/afkham_azeez*http://twitter.com/afkham_azeez
*
linked-in: **http://lk.linkedin.com/in/afkhamazeez*
*
*
*Lean . Enterprise . Middleware*
--
Thanks regards,
Nirmal
Senior Software Engineer- Platform Technologies Team, WSO2 Inc.
Mobile: +94715779733
Blog: http://nirmalfdo.blogspot.com/
___
Dev mailing list
Dev@wso2.org
http://wso2.org/cgi-bin/mailman/listinfo/dev
--
*Kishanthan Thangarajah*
Senior Software Engineer,
Platform Technologies Team,
WSO2, Inc.
lean.enterprise.middleware
Mobile - +94773426635
Blog - *http://kishanthan.wordpress.com*
Twitter - *http://twitter.com/kishanthan*
___
Dev mailing list
Dev@wso2.org
http://wso2.org/cgi-bin/mailman/listinfo/dev
Re: [Dev] Debug statements in TenantAwareLoadBalanceEndpoint
Hi Sameera, Please apply the attached patch to https://svn.wso2.org/repos/wso2/carbon/platform/trunk/components/load-balancer/lb-endpoint/org.wso2.carbon.lb.endpoint On Fri, Aug 16, 2013 at 9:39 PM, Lahiru Sandaruwan lahi...@wso2.com wrote: Hi, On Fri, Aug 16, 2013 at 8:31 PM, Nirmal Fernando nir...@wso2.com wrote: Hi, On Fri, Aug 16, 2013 at 7:43 PM, Sameera Jayasoma same...@wso2.comwrote: I've also seen some place where we need to improve in the TenantAwareLoadBalanceEndpoint class. Here are my observations. 1) Five different inner for-loops. This is a bad smell in the code. This five different inner 'for-loops' are not in TenantAwareLoadBalanceEndpoint class but in 'LoadBalancerConfiguration' file. And on the top of the method that has five different inner loops, there's a comment as follows (with a FIXME tag): *// FIXME if possible! I couldn't think of any other way to do this, at this moment. // Note: some of these for-loops are pretty small, thus no considerable performance overhead.* So, as the comment says, +1 for re-visiting the code and fixing it. 2) We do a registry lookup for each and every unknown host name. This bit of code is vulnerable to a DOS Attack. Yes, Understood. Since the newly mapped domains are added to registry dynamically, we have to look whether the invalid domain is defined there(In Stratos domain mapping scenario). Possible solution would be keeping the domain mapping in memory and persist(For a failover/HA scenario). For that, we need to do web service call to ELB or trigger an even to notify ELB. But service hosting facility is disabled in ELB AFAIR. So we might be able to use queue subscription to pass the message which we already have. That way we can avoid using registry and also can gain better performance. Please explain if there is a better way? @Lahiru please explain the context behind this. 3) This code looks more like procedural-oriented code. You mean the code in this class?? Lets do a proper code review on this and refactor the code for the next release. I've created a Redmine task for this. +1. We need all the people who contributed to this code, to understand the rationale behind. Thanks, Sameera. On Fri, Aug 16, 2013 at 4:43 PM, Afkham Azeez az...@wso2.com wrote: There are a number of debug statements which do String concatenations, and are not wrapped in log.isDebugEnabled() conditions. Such statements will cause unnecessary performance overhead. -- *Afkham Azeez* Director of Architecture; WSO2, Inc.; http://wso2.com Member; Apache Software Foundation; http://www.apache.org/ * http://www.apache.org/** email: **az...@wso2.com* az...@wso2.com* cell: +94 77 3320919 blog: **http://blog.afkham.org* http://blog.afkham.org* twitter: **http://twitter.com/afkham_azeez*http://twitter.com/afkham_azeez * linked-in: **http://lk.linkedin.com/in/afkhamazeez* * * *Lean . Enterprise . Middleware* -- Sameera Jayasoma, Architect, WSO2, Inc. (http://wso2.com) email: same...@wso2.com blog: http://sameera.adahas.org twitter: https://twitter.com/sameerajayasoma flickr: http://www.flickr.com/photos/sameera-jayasoma/collections Lean . Enterprise . Middleware -- Thanks regards, Nirmal Senior Software Engineer- Platform Technologies Team, WSO2 Inc. Mobile: +94715779733 Blog: http://nirmalfdo.blogspot.com/ -- -- Lahiru Sandaruwan Software Engineer, Platform Technologies, WSO2 Inc., http://wso2.com lean.enterprise.middleware email: lahi...@wso2.com cell: (+94) 773 325 954 blog: http://lahiruwrites.blogspot.com/ twitter: http://twitter.com/lahirus linked-in: http://lk.linkedin.com/pub/lahiru-sandaruwan/16/153/146 -- Thanks regards, Nirmal Senior Software Engineer- Platform Technologies Team, WSO2 Inc. Mobile: +94715779733 Blog: http://nirmalfdo.blogspot.com/ wrapDebugLogs.diff Description: Binary data ___ Dev mailing list Dev@wso2.org http://wso2.org/cgi-bin/mailman/listinfo/dev
[Dev] Debug statements in TenantAwareLoadBalanceEndpoint
There are a number of debug statements which do String concatenations, and are not wrapped in log.isDebugEnabled() conditions. Such statements will cause unnecessary performance overhead. -- *Afkham Azeez* Director of Architecture; WSO2, Inc.; http://wso2.com Member; Apache Software Foundation; http://www.apache.org/ * http://www.apache.org/** email: **az...@wso2.com* az...@wso2.com* cell: +94 77 3320919 blog: **http://blog.afkham.org* http://blog.afkham.org* twitter: **http://twitter.com/afkham_azeez*http://twitter.com/afkham_azeez * linked-in: **http://lk.linkedin.com/in/afkhamazeez* * * *Lean . Enterprise . Middleware* ___ Dev mailing list Dev@wso2.org http://wso2.org/cgi-bin/mailman/listinfo/dev
Re: [Dev] Debug statements in TenantAwareLoadBalanceEndpoint
I've also seen some place where we need to improve in the TenantAwareLoadBalanceEndpoint class. Here are my observations. 1) Five different inner for-loops. This is a bad smell in the code. 2) We do a registry lookup for each and every unknown host name. This bit of code is vulnerable to a DOS Attack. 3) This code looks more like procedural-oriented code. Lets do a proper code review on this and refactor the code for the next release. I've created a Redmine task for this. Thanks, Sameera. On Fri, Aug 16, 2013 at 4:43 PM, Afkham Azeez az...@wso2.com wrote: There are a number of debug statements which do String concatenations, and are not wrapped in log.isDebugEnabled() conditions. Such statements will cause unnecessary performance overhead. -- *Afkham Azeez* Director of Architecture; WSO2, Inc.; http://wso2.com Member; Apache Software Foundation; http://www.apache.org/ * http://www.apache.org/** email: **az...@wso2.com* az...@wso2.com* cell: +94 77 3320919 blog: **http://blog.afkham.org* http://blog.afkham.org* twitter: **http://twitter.com/afkham_azeez*http://twitter.com/afkham_azeez * linked-in: **http://lk.linkedin.com/in/afkhamazeez* * * *Lean . Enterprise . Middleware* -- Sameera Jayasoma, Architect, WSO2, Inc. (http://wso2.com) email: same...@wso2.com blog: http://sameera.adahas.org twitter: https://twitter.com/sameerajayasoma flickr: http://www.flickr.com/photos/sameera-jayasoma/collections Lean . Enterprise . Middleware ___ Dev mailing list Dev@wso2.org http://wso2.org/cgi-bin/mailman/listinfo/dev
Re: [Dev] Debug statements in TenantAwareLoadBalanceEndpoint
Hi, On Fri, Aug 16, 2013 at 7:43 PM, Sameera Jayasoma same...@wso2.com wrote: I've also seen some place where we need to improve in the TenantAwareLoadBalanceEndpoint class. Here are my observations. 1) Five different inner for-loops. This is a bad smell in the code. This five different inner 'for-loops' are not in TenantAwareLoadBalanceEndpoint class but in 'LoadBalancerConfiguration' file. And on the top of the method that has five different inner loops, there's a comment as follows (with a FIXME tag): *// FIXME if possible! I couldn't think of any other way to do this, at this moment. // Note: some of these for-loops are pretty small, thus no considerable performance overhead.* So, as the comment says, +1 for re-visiting the code and fixing it. 2) We do a registry lookup for each and every unknown host name. This bit of code is vulnerable to a DOS Attack. @Lahiru please explain the context behind this. 3) This code looks more like procedural-oriented code. You mean the code in this class?? Lets do a proper code review on this and refactor the code for the next release. I've created a Redmine task for this. +1. We need all the people who contributed to this code, to understand the rationale behind. Thanks, Sameera. On Fri, Aug 16, 2013 at 4:43 PM, Afkham Azeez az...@wso2.com wrote: There are a number of debug statements which do String concatenations, and are not wrapped in log.isDebugEnabled() conditions. Such statements will cause unnecessary performance overhead. -- *Afkham Azeez* Director of Architecture; WSO2, Inc.; http://wso2.com Member; Apache Software Foundation; http://www.apache.org/ * http://www.apache.org/** email: **az...@wso2.com* az...@wso2.com* cell: +94 77 3320919 blog: **http://blog.afkham.org* http://blog.afkham.org* twitter: **http://twitter.com/afkham_azeez*http://twitter.com/afkham_azeez * linked-in: **http://lk.linkedin.com/in/afkhamazeez* * * *Lean . Enterprise . Middleware* -- Sameera Jayasoma, Architect, WSO2, Inc. (http://wso2.com) email: same...@wso2.com blog: http://sameera.adahas.org twitter: https://twitter.com/sameerajayasoma flickr: http://www.flickr.com/photos/sameera-jayasoma/collections Lean . Enterprise . Middleware -- Thanks regards, Nirmal Senior Software Engineer- Platform Technologies Team, WSO2 Inc. Mobile: +94715779733 Blog: http://nirmalfdo.blogspot.com/ ___ Dev mailing list Dev@wso2.org http://wso2.org/cgi-bin/mailman/listinfo/dev
Re: [Dev] Debug statements in TenantAwareLoadBalanceEndpoint
Hi Azeez,
I grepped the class:
nirmal@nirmal:/media/wso2/carbon/platform/trunk/components/load-balancer/lb-endpoint/org.wso2.carbon.lb.endpoint$
grep -nr log.isDebug
./src/main/java/org/wso2/carbon/lb/endpoint/endpoint/TenantAwareLoadBalanceEndpoint.java
142:if (log.isDebugEnabled()) {
268:if (log.isDebugEnabled()) {
nirmal@nirmal:/media/wso2/carbon/platform/trunk/components/load-balancer/lb-endpoint/org.wso2.carbon.lb.endpoint$
nirmal@nirmal:/media/wso2/carbon/platform/trunk/components/load-balancer/lb-endpoint/org.wso2.carbon.lb.endpoint$
nirmal@nirmal:/media/wso2/carbon/platform/trunk/components/load-balancer/lb-endpoint/org.wso2.carbon.lb.endpoint$
nirmal@nirmal:/media/wso2/carbon/platform/trunk/components/load-balancer/lb-endpoint/org.wso2.carbon.lb.endpoint$
nirmal@nirmal:/media/wso2/carbon/platform/trunk/components/load-balancer/lb-endpoint/org.wso2.carbon.lb.endpoint$
nirmal@nirmal:/media/wso2/carbon/platform/trunk/components/load-balancer/lb-endpoint/org.wso2.carbon.lb.endpoint$
grep -nr log.debug
./src/main/java/org/wso2/carbon/lb/endpoint/endpoint/TenantAwareLoadBalanceEndpoint.java
143:log.debug(Group management agent added
to cluster domain: +
269:log.debug(Current session id : +
sessionInformation.getId());
299:log.debug(* Actual Host: +actualHost + **
Target Host: +targetHost);
328:log.debug(* Actual Host: +actualHost
+ ** Target Host: +targetHost);
431:log.debug(Setting host name: +host);
454:log.debug(Retrying to a failed member has stopped.);
nirmal@nirmal
:/media/wso2/carbon/platform/trunk/components/load-balancer/lb-endpoint/org.wso2.carbon.lb.endpoint$
Those in green are wrapped using log.isDebugEnabled.
One in beige has no concatenation.
I guess the concern is on ones in red. +1 for wrapping those.
On Fri, Aug 16, 2013 at 4:43 PM, Afkham Azeez az...@wso2.com wrote:
There are a number of debug statements which do String concatenations, and
are not wrapped in log.isDebugEnabled() conditions. Such statements will
cause unnecessary performance overhead.
--
*Afkham Azeez*
Director of Architecture; WSO2, Inc.; http://wso2.com
Member; Apache Software Foundation; http://www.apache.org/
* http://www.apache.org/**
email: **az...@wso2.com* az...@wso2.com* cell: +94 77 3320919
blog: **http://blog.afkham.org* http://blog.afkham.org*
twitter: **http://twitter.com/afkham_azeez*http://twitter.com/afkham_azeez
*
linked-in: **http://lk.linkedin.com/in/afkhamazeez*
*
*
*Lean . Enterprise . Middleware*
--
Thanks regards,
Nirmal
Senior Software Engineer- Platform Technologies Team, WSO2 Inc.
Mobile: +94715779733
Blog: http://nirmalfdo.blogspot.com/
___
Dev mailing list
Dev@wso2.org
http://wso2.org/cgi-bin/mailman/listinfo/dev
Re: [Dev] Debug statements in TenantAwareLoadBalanceEndpoint
Hi, On Fri, Aug 16, 2013 at 8:31 PM, Nirmal Fernando nir...@wso2.com wrote: Hi, On Fri, Aug 16, 2013 at 7:43 PM, Sameera Jayasoma same...@wso2.comwrote: I've also seen some place where we need to improve in the TenantAwareLoadBalanceEndpoint class. Here are my observations. 1) Five different inner for-loops. This is a bad smell in the code. This five different inner 'for-loops' are not in TenantAwareLoadBalanceEndpoint class but in 'LoadBalancerConfiguration' file. And on the top of the method that has five different inner loops, there's a comment as follows (with a FIXME tag): *// FIXME if possible! I couldn't think of any other way to do this, at this moment. // Note: some of these for-loops are pretty small, thus no considerable performance overhead.* So, as the comment says, +1 for re-visiting the code and fixing it. 2) We do a registry lookup for each and every unknown host name. This bit of code is vulnerable to a DOS Attack. Yes, Understood. Since the newly mapped domains are added to registry dynamically, we have to look whether the invalid domain is defined there(In Stratos domain mapping scenario). Possible solution would be keeping the domain mapping in memory and persist(For a failover/HA scenario). For that, we need to do web service call to ELB or trigger an even to notify ELB. But service hosting facility is disabled in ELB AFAIR. So we might be able to use queue subscription to pass the message which we already have. That way we can avoid using registry and also can gain better performance. Please explain if there is a better way? @Lahiru please explain the context behind this. 3) This code looks more like procedural-oriented code. You mean the code in this class?? Lets do a proper code review on this and refactor the code for the next release. I've created a Redmine task for this. +1. We need all the people who contributed to this code, to understand the rationale behind. Thanks, Sameera. On Fri, Aug 16, 2013 at 4:43 PM, Afkham Azeez az...@wso2.com wrote: There are a number of debug statements which do String concatenations, and are not wrapped in log.isDebugEnabled() conditions. Such statements will cause unnecessary performance overhead. -- *Afkham Azeez* Director of Architecture; WSO2, Inc.; http://wso2.com Member; Apache Software Foundation; http://www.apache.org/ * http://www.apache.org/** email: **az...@wso2.com* az...@wso2.com* cell: +94 77 3320919 blog: **http://blog.afkham.org* http://blog.afkham.org* twitter: **http://twitter.com/afkham_azeez*http://twitter.com/afkham_azeez * linked-in: **http://lk.linkedin.com/in/afkhamazeez* * * *Lean . Enterprise . Middleware* -- Sameera Jayasoma, Architect, WSO2, Inc. (http://wso2.com) email: same...@wso2.com blog: http://sameera.adahas.org twitter: https://twitter.com/sameerajayasoma flickr: http://www.flickr.com/photos/sameera-jayasoma/collections Lean . Enterprise . Middleware -- Thanks regards, Nirmal Senior Software Engineer- Platform Technologies Team, WSO2 Inc. Mobile: +94715779733 Blog: http://nirmalfdo.blogspot.com/ -- -- Lahiru Sandaruwan Software Engineer, Platform Technologies, WSO2 Inc., http://wso2.com lean.enterprise.middleware email: lahi...@wso2.com cell: (+94) 773 325 954 blog: http://lahiruwrites.blogspot.com/ twitter: http://twitter.com/lahirus linked-in: http://lk.linkedin.com/pub/lahiru-sandaruwan/16/153/146 ___ Dev mailing list Dev@wso2.org http://wso2.org/cgi-bin/mailman/listinfo/dev
