You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@ofbiz.apache.org by mb...@apache.org on 2017/10/10 13:46:43 UTC
svn commit: r1811699 -
/ofbiz/ofbiz-framework/trunk/framework/service/src/main/java/org/apache/ofbiz/service/config/model/ThreadPool.java
Author: mbrohl
Date: Tue Oct 10 13:46:42 2017
New Revision: 1811699
URL: http://svn.apache.org/viewvc?rev=1811699&view=rev
Log:
Improved: Fixing defects reported by FindBugs, package
org.apache.ofbiz.service.config.model.
(OFBIZ-9682)
Thanks Julian Leichert for reporting and providing the patch.
Modified:
ofbiz/ofbiz-framework/trunk/framework/service/src/main/java/org/apache/ofbiz/service/config/model/ThreadPool.java
Modified: ofbiz/ofbiz-framework/trunk/framework/service/src/main/java/org/apache/ofbiz/service/config/model/ThreadPool.java
URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/framework/service/src/main/java/org/apache/ofbiz/service/config/model/ThreadPool.java?rev=1811699&r1=1811698&r2=1811699&view=diff
==============================================================================
--- ofbiz/ofbiz-framework/trunk/framework/service/src/main/java/org/apache/ofbiz/service/config/model/ThreadPool.java (original)
+++ ofbiz/ofbiz-framework/trunk/framework/service/src/main/java/org/apache/ofbiz/service/config/model/ThreadPool.java Tue Oct 10 13:46:42 2017
@@ -23,6 +23,7 @@ import java.util.Collections;
import java.util.List;
import org.apache.ofbiz.base.lang.ThreadSafe;
+import org.apache.ofbiz.base.util.Debug;
import org.apache.ofbiz.base.util.UtilXml;
import org.apache.ofbiz.service.config.ServiceConfigException;
import org.w3c.dom.Element;
@@ -33,6 +34,8 @@ import org.w3c.dom.Element;
@ThreadSafe
public final class ThreadPool {
+ private static final String module = ThreadPool.class.getName();
+
public static final int FAILED_RETRY_MIN = 30;
public static final int MIN_THREADS = 1; // Must be no less than one or the executor will shut down.
public static final int MAX_THREADS = 5; // Values higher than 5 might slow things down.
@@ -52,7 +55,7 @@ public final class ThreadPool {
private final String sendToPool;
private final int ttl;
- ThreadPool(Element poolElement) throws ServiceConfigException {
+ ThreadPool(Element poolElement) throws ServiceConfigException, NumberFormatException {
String sendToPool = poolElement.getAttribute("send-to-pool").intern();
if (sendToPool.isEmpty()) {
throw new ServiceConfigException("<thread-pool> element send-to-pool attribute is empty");
@@ -67,7 +70,7 @@ public final class ThreadPool {
if (this.purgeJobDays < 0) {
throw new ServiceConfigException("<thread-pool> element purge-job-days attribute value is invalid");
}
- } catch (Exception e) {
+ } catch (NumberFormatException | ServiceConfigException e) {
throw new ServiceConfigException("<thread-pool> element purge-job-days attribute value is invalid");
}
}
@@ -80,7 +83,8 @@ public final class ThreadPool {
if (this.failedRetryMin < 0) {
throw new ServiceConfigException("<thread-pool> element failed-retry-min attribute value is invalid");
}
- } catch (Exception e) {
+ } catch (NumberFormatException | ServiceConfigException e) {
+ Debug.logError(e, module);
throw new ServiceConfigException("<thread-pool> element failed-retry-min attribute value is invalid");
}
}
@@ -93,7 +97,8 @@ public final class ThreadPool {
if (this.ttl < 0) {
throw new ServiceConfigException("<thread-pool> element ttl attribute value is invalid");
}
- } catch (Exception e) {
+ } catch (NumberFormatException | ServiceConfigException e) {
+ Debug.logError(e, module);
throw new ServiceConfigException("<thread-pool> element ttl attribute value is invalid");
}
}
@@ -106,7 +111,8 @@ public final class ThreadPool {
if (this.jobs < 1) {
throw new ServiceConfigException("<thread-pool> element jobs attribute value is invalid");
}
- } catch (Exception e) {
+ } catch (NumberFormatException | ServiceConfigException e) {
+ Debug.logError(e, module);
throw new ServiceConfigException("<thread-pool> element jobs attribute value is invalid");
}
}
@@ -119,7 +125,8 @@ public final class ThreadPool {
if (this.minThreads < 1) {
throw new ServiceConfigException("<thread-pool> element min-threads attribute value is invalid");
}
- } catch (Exception e) {
+ } catch (NumberFormatException | ServiceConfigException e) {
+ Debug.logError(e, module);
throw new ServiceConfigException("<thread-pool> element min-threads attribute value is invalid");
}
}
@@ -132,7 +139,8 @@ public final class ThreadPool {
if (this.maxThreads < this.minThreads) {
throw new ServiceConfigException("<thread-pool> element max-threads attribute value is invalid");
}
- } catch (Exception e) {
+ } catch (NumberFormatException | ServiceConfigException e) {
+ Debug.logError(e, module);
throw new ServiceConfigException("<thread-pool> element max-threads attribute value is invalid");
}
}
@@ -146,7 +154,8 @@ public final class ThreadPool {
if (this.pollDbMillis < 0) {
throw new ServiceConfigException("<thread-pool> element poll-db-millis attribute value is invalid");
}
- } catch (Exception e) {
+ } catch (NumberFormatException | ServiceConfigException e) {
+ Debug.logError(e, module);
throw new ServiceConfigException("<thread-pool> element poll-db-millis attribute value is invalid");
}
}
Re: svn commit: r1811699 -
/ofbiz/ofbiz-framework/trunk/framework/service/src/main/java/org/apache/ofb
iz/service/config/model/ThreadPool.java
Posted by Jacques Le Roux <ja...@les7arts.com>.
Hi Scott, Michael,
It's late but I'm still reading Java Concurrency in Practice (not now, but actually often :)) and I agree with (all) answers at
https://stackoverflow.com/questions/13834692/threads-configuration-based-on-no-of-cpu-cores
It's indeed not a trivial task to determine the best value, and the reference above in serviceengine.xml should not hurt.
We can keep 5 but I believe have to explain why at the upper level. It's a very important value and not all people (often) read Java Concurrency in
Practice
What do you think?
Jacques
Le 12/10/2017 à 22:12, Scott Gray a écrit :
> Honestly I think the topic is generic enough that OFBiz doesn't need to
> provide any information at all. Thread pool sizing is not exclusive to
> OFBiz and it would be strange for anyone to modify the numbers without
> first researching sources that provide far more detail than a few sentences
> in our config files will ever cover.
>
> Regards
> Scott
>
> On 12 October 2017 at 12:49, Michael Brohl <mi...@ecomify.de> wrote:
>
>> Hi Jacques,
>>
>> yes, in general I think it's best to document at the place where users do
>> the configuration.
>>
>> In this case, I cannot say if the statement makes any sense. It can be
>> from ancient times where you only had single core systems not making sense
>> anymore and it can also be correct today.
>>
>> Maybe someone with more knowledege in this area can give us more info.
>>
>> Regards,
>>
>> Michael
>>
>>
>> Am 12.10.17 um 14:17 schrieb Jacques Le Roux:
>>
>> Right, thanks Michael
>>> Thought some documentation is present in ThreadPool.java, I think we
>>> should document that at the service serviceengine.xml level
>>>
>>> I mean, in general, we should document at the level used to configure,
>>> not below.
>>>
>>> Here for instance something like "Values higher than 5 might slow things
>>> down depending of your cores numbers"
>>>
>>> Also before written so we would have to verify why the 5 value was picked
>>>
>>> What do you think?
>>>
>>> Jacques
>>>
>>>
>>> Le 12/10/2017 à 13:03, Michael Brohl a écrit :
>>>
>>>> Hi Jacques,
>>>>
>>>> as you said, it's just a default and the value can be configured through
>>>> "max-threads".
>>>>
>>>> I would leave it as is.
>>>>
>>>> Regards,
>>>>
>>>> Michael
>>>>
>>>>
>>>> Am 11.10.17 um 07:58 schrieb Jacques Le Roux:
>>>>
>>>>> MAX_THREADS = 5
>>>>>
>>>>
>>>>
>>
Re: svn commit: r1811699 -
/ofbiz/ofbiz-framework/trunk/framework/service/src/main/java/org/apache/ofb
iz/service/config/model/ThreadPool.java
Posted by Jacques Le Roux <ja...@les7arts.com>.
Mmm, it seems I wanted to say something here and pushed a button too fast :D
Actually I guess you saw this was done very simply at r1817040
Jacques
Le 24/11/2017 à 19:34, Jacques Le Roux a écrit :
>
>
>
> Le 12/10/2017 à 22:12, Scott Gray a écrit :
>> Honestly I think the topic is generic enough that OFBiz doesn't need to
>> provide any information at all. Thread pool sizing is not exclusive to
>> OFBiz and it would be strange for anyone to modify the numbers without
>> first researching sources that provide far more detail than a few sentences
>> in our config files will ever cover.
>>
>> Regards
>> Scott
>>
>> On 12 October 2017 at 12:49, Michael Brohl<mi...@ecomify.de> wrote:
>>
>>> Hi Jacques,
>>>
>>> yes, in general I think it's best to document at the place where users do
>>> the configuration.
>>>
>>> In this case, I cannot say if the statement makes any sense. It can be
>>> from ancient times where you only had single core systems not making sense
>>> anymore and it can also be correct today.
>>>
>>> Maybe someone with more knowledege in this area can give us more info.
>>>
>>> Regards,
>>>
>>> Michael
>>>
>>>
>>> Am 12.10.17 um 14:17 schrieb Jacques Le Roux:
>>>
>>> Right, thanks Michael
>>>> Thought some documentation is present in ThreadPool.java, I think we
>>>> should document that at the service serviceengine.xml level
>>>>
>>>> I mean, in general, we should document at the level used to configure,
>>>> not below.
>>>>
>>>> Here for instance something like "Values higher than 5 might slow things
>>>> down depending of your cores numbers"
>>>>
>>>> Also before written so we would have to verify why the 5 value was picked
>>>>
>>>> What do you think?
>>>>
>>>> Jacques
>>>>
>>>>
>>>> Le 12/10/2017 à 13:03, Michael Brohl a écrit :
>>>>
>>>>> Hi Jacques,
>>>>>
>>>>> as you said, it's just a default and the value can be configured through
>>>>> "max-threads".
>>>>>
>>>>> I would leave it as is.
>>>>>
>>>>> Regards,
>>>>>
>>>>> Michael
>>>>>
>>>>>
>>>>> Am 11.10.17 um 07:58 schrieb Jacques Le Roux:
>>>>>
>>>>>> MAX_THREADS = 5
>>>>>>
>>>>>
>
Re: svn commit: r1811699 - /ofbiz/ofbiz-framework/trunk/framework/service/src/main/java/org/apache/ofb
iz/service/config/model/ThreadPool.java
Posted by Scott Gray <sc...@hotwaxsystems.com>.
Honestly I think the topic is generic enough that OFBiz doesn't need to
provide any information at all. Thread pool sizing is not exclusive to
OFBiz and it would be strange for anyone to modify the numbers without
first researching sources that provide far more detail than a few sentences
in our config files will ever cover.
Regards
Scott
On 12 October 2017 at 12:49, Michael Brohl <mi...@ecomify.de> wrote:
> Hi Jacques,
>
> yes, in general I think it's best to document at the place where users do
> the configuration.
>
> In this case, I cannot say if the statement makes any sense. It can be
> from ancient times where you only had single core systems not making sense
> anymore and it can also be correct today.
>
> Maybe someone with more knowledege in this area can give us more info.
>
> Regards,
>
> Michael
>
>
> Am 12.10.17 um 14:17 schrieb Jacques Le Roux:
>
> Right, thanks Michael
>>
>> Thought some documentation is present in ThreadPool.java, I think we
>> should document that at the service serviceengine.xml level
>>
>> I mean, in general, we should document at the level used to configure,
>> not below.
>>
>> Here for instance something like "Values higher than 5 might slow things
>> down depending of your cores numbers"
>>
>> Also before written so we would have to verify why the 5 value was picked
>>
>> What do you think?
>>
>> Jacques
>>
>>
>> Le 12/10/2017 à 13:03, Michael Brohl a écrit :
>>
>>> Hi Jacques,
>>>
>>> as you said, it's just a default and the value can be configured through
>>> "max-threads".
>>>
>>> I would leave it as is.
>>>
>>> Regards,
>>>
>>> Michael
>>>
>>>
>>> Am 11.10.17 um 07:58 schrieb Jacques Le Roux:
>>>
>>>> MAX_THREADS = 5
>>>>
>>>
>>>
>>>
>>
>
>
Re: svn commit: r1811699 -
/ofbiz/ofbiz-framework/trunk/framework/service/src/main/java/org/apache/ofb
iz/service/config/model/ThreadPool.java
Posted by Michael Brohl <mi...@ecomify.de>.
Hi Jacques,
yes, in general I think it's best to document at the place where users
do the configuration.
In this case, I cannot say if the statement makes any sense. It can be
from ancient times where you only had single core systems not making
sense anymore and it can also be correct today.
Maybe someone with more knowledege in this area can give us more info.
Regards,
Michael
Am 12.10.17 um 14:17 schrieb Jacques Le Roux:
> Right, thanks Michael
>
> Thought some documentation is present in ThreadPool.java, I think we
> should document that at the service serviceengine.xml level
>
> I mean, in general, we should document at the level used to configure,
> not below.
>
> Here for instance something like "Values higher than 5 might slow
> things down depending of your cores numbers"
>
> Also before written so we would have to verify why the 5 value was picked
>
> What do you think?
>
> Jacques
>
>
> Le 12/10/2017 à 13:03, Michael Brohl a écrit :
>> Hi Jacques,
>>
>> as you said, it's just a default and the value can be configured
>> through "max-threads".
>>
>> I would leave it as is.
>>
>> Regards,
>>
>> Michael
>>
>>
>> Am 11.10.17 um 07:58 schrieb Jacques Le Roux:
>>> MAX_THREADS = 5
>>
>>
>
Re: svn commit: r1811699 -
/ofbiz/ofbiz-framework/trunk/framework/service/src/main/java/org/apache/ofb
iz/service/config/model/ThreadPool.java
Posted by Jacques Le Roux <ja...@les7arts.com>.
Right, thanks Michael
Thought some documentation is present in ThreadPool.java, I think we should document that at the service serviceengine.xml level
I mean, in general, we should document at the level used to configure, not below.
Here for instance something like "Values higher than 5 might slow things down depending of your cores numbers"
Also before written so we would have to verify why the 5 value was picked
What do you think?
Jacques
Le 12/10/2017 à 13:03, Michael Brohl a écrit :
> Hi Jacques,
>
> as you said, it's just a default and the value can be configured through "max-threads".
>
> I would leave it as is.
>
> Regards,
>
> Michael
>
>
> Am 11.10.17 um 07:58 schrieb Jacques Le Roux:
>> MAX_THREADS = 5
>
>
Re: svn commit: r1811699 -
/ofbiz/ofbiz-framework/trunk/framework/service/src/main/java/org/apache/ofb
iz/service/config/model/ThreadPool.java
Posted by Michael Brohl <mi...@ecomify.de>.
Hi Jacques,
as you said, it's just a default and the value can be configured through
"max-threads".
I would leave it as is.
Regards,
Michael
Am 11.10.17 um 07:58 schrieb Jacques Le Roux:
> MAX_THREADS = 5
Re: svn commit: r1811699 -
/ofbiz/ofbiz-framework/trunk/framework/service/src/main/java/org/apache/ofb
iz/service/config/model/ThreadPool.java
Posted by Jacques Le Roux <ja...@les7arts.com>.
Le 10/10/2017 à 15:46, mbrohl@apache.org a écrit :
> public static final int MAX_THREADS = 5; // Values higher than 5 might slow things down.
Hi
While reviewing recent Michael's commits (thanks Michael and the ecomify team) I stumbled upon this static variable
I know it's only a default, but yet should it not be dynamic in relation with the number of cores?
Disclaimer: I did not dig further so ignore the question if it's non sense
Jacques