[Scheduler] Dropped usage of SimpleDateFormat for trace logging #3508

sjsf · 2017-05-23T12:49:26Z

...as it slows down the scheduler and was used in a dangeriously
wrt thread-safety.

fixes #3505
fixes #3506
Signed-off-by: Simon Kaufmann simon.kfm@googlemail.com

htreu · 2017-05-23T14:02:24Z

...ome.core/src/main/java/org/eclipse/smarthome/core/scheduler/ExpressionThreadPoolManager.java

@@ -38,8 +37,6 @@

 private final static Logger logger = LoggerFactory.getLogger(ExpressionThreadPoolManager.class);


I know its not part of the PR but maybe you can change the logger to privat final right away.

I feared this would catch sb's eye 😉
No, it's not that easy, but will do so.

Then let's do it in another PR as this change is not related at all. Okay?

maggu2810

Thank you for that changes.
Perhaps you can also add the change for the comment of @htreu

sjsf · 2017-05-23T15:16:28Z

done

maggu2810

@SJKA I found a few other things on review.
The question about the class used for the logger is really affected by changes of this PR.
Some other comments has been added because I found on while looking at the code.

maggu2810 · 2017-05-24T05:28:11Z

...ome.core/src/main/java/org/eclipse/smarthome/core/scheduler/ExpressionThreadPoolManager.java

@@ -86,7 +84,8 @@ public ExpressionThreadPoolExecutor(final String poolName, int corePoolSize) {
 @Override
 public void rejectedExecution(Runnable runnable, ThreadPoolExecutor threadPoolExecutor) {
 // Log and discard
- logger.warn("Thread pool '{}' rejected execution of {}",
+ LoggerFactory.getLogger(ExpressionThreadPoolExecutor.class).warn(


As we create here a implementation of the ThreadPoolExecutor.DiscardPolicy class, shouldn't we add a logger to that implementation using the class it implements?

maggu2810 · 2017-05-24T05:28:44Z

...ome.core/src/main/java/org/eclipse/smarthome/core/scheduler/ExpressionThreadPoolManager.java

@@ -86,7 +84,8 @@ public ExpressionThreadPoolExecutor(final String poolName, int corePoolSize) {
 @Override
 public void rejectedExecution(Runnable runnable, ThreadPoolExecutor threadPoolExecutor) {
 // Log and discard
- logger.warn("Thread pool '{}' rejected execution of {}",
+ LoggerFactory.getLogger(ExpressionThreadPoolExecutor.class).warn(
+ "Thread pool '{}' rejected execution of {}",
 new Object[] { poolName, runnable.getClass() });


Not introduced by your changes, but do we need to create an array of the two logger arguments?

done - plus I reduced the severity to DEBUG as the comment suggests this actually is expected to happen during shutdown.

maggu2810 · 2017-05-24T05:30:02Z

...ome.core/src/main/java/org/eclipse/smarthome/core/scheduler/ExpressionThreadPoolManager.java

@@ -59,7 +54,8 @@ static public ExpressionThreadPoolExecutor getExpressionScheduledPool(String poo
 ((ThreadPoolExecutor) pool).setKeepAliveTime(THREAD_TIMEOUT, TimeUnit.SECONDS);
 ((ThreadPoolExecutor) pool).allowCoreThreadTimeOut(true);
 pools.put(poolName, pool);
- logger.debug("Created an expression-drive scheduled thread pool '{}' of size {}",
+ LoggerFactory.getLogger(ExpressionThreadPoolManager.class).debug(


Also not really part of this PR: If and only if you do another change again, WDYT about changing static public to public static to get the common keyword order?

maggu2810 · 2017-05-24T05:30:24Z

...ome.core/src/main/java/org/eclipse/smarthome/core/scheduler/ExpressionThreadPoolManager.java

@@ -59,7 +54,8 @@ static public ExpressionThreadPoolExecutor getExpressionScheduledPool(String poo
 ((ThreadPoolExecutor) pool).setKeepAliveTime(THREAD_TIMEOUT, TimeUnit.SECONDS);
 ((ThreadPoolExecutor) pool).allowCoreThreadTimeOut(true);
 pools.put(poolName, pool);
- logger.debug("Created an expression-drive scheduled thread pool '{}' of size {}",
+ LoggerFactory.getLogger(ExpressionThreadPoolManager.class).debug(
+ "Created an expression-drive scheduled thread pool '{}' of size {}",
 new Object[] { poolName, cfg });


Also not really part of this PR: Why using an array for the arguments?

...as it slows down the scheduler and was used in a dangeriously wrt thread-safety. fixes eclipse-archived#3505 fixes eclipse-archived#3506 Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>

amitjoy · 2017-05-24T08:45:30Z

...ome.core/src/main/java/org/eclipse/smarthome/core/scheduler/ExpressionThreadPoolManager.java

@@ -36,18 +35,14 @@
 */
 public class ExpressionThreadPoolManager extends ThreadPoolManager {

- private final static Logger logger = LoggerFactory.getLogger(ExpressionThreadPoolManager.class);


Instead of using logger for every inner class, you could also do this:

private final Logger logger = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());

This will be automatically resolved for every class instance that uses it.

Are you really sure that this will give a different logger class when calling the same instance from the different locations, I cannot see how that would work out. The lookup is done only once when initializing.

I actually use this in inheritance pattern in which the logger is mentioned in interface or base (abstract) class so that the logger gets resolved for every child class instances. I completely overlooked in this scenario that it will be resolved only for the top-level class during instantiation and the same logger instance will be used by the inner classes.

* master: (335 commits) Voice: send feedback to an item when listening for a command (eclipse-archived#3451) Scheduler: fix ConcurrentModificationException (eclipse-archived#3511) [Scheduler] Dropped usage of SimpleDateFormat for trace logging (eclipse-archived#3508) Use % sign as default for dimmer items (eclipse-archived#3525) updated ThingHandler javadoc added regression tests ensure thingUdpated cannot be called in parallel with initialize moving handler intiialization back to the caller thread use decicated locks instead of syncrhonization by object add PaperUI setup to IDE setup tasks, adopt documentation (eclipse-archived#3515) Bug fix: things not showing on control page (eclipse-archived#3517) remove "Type" postfix in item events, fixes 3282. (eclipse-archived#3494) use toFullString when creating GroupItemStateChangeEvent (eclipse-archived#3409) Made item’s state text consistent + refactored code (eclipse-archived#3466) Refactored CoreItemFactory: (eclipse-archived#3507) Add CoreItemFactoryTest, in addition to eclipse-archived#3507 (eclipse-archived#3509) fix ConcurrentModificationException in AbstractWatchServiceTest (eclipse-archived#3499) BasicUI: Treat Switch on NumberItem not as ON/OFF Switch (eclipse-archived#3493) An Interface method should not allow for throwing a generic Exception (eclipse-archived#3467) Fix eclipse-archived#2902 for Basic UI (eclipse-archived#3496) ... # Conflicts: # bundles/io/org.eclipse.smarthome.io.transport.dbus/.classpath # bundles/io/org.eclipse.smarthome.io.transport.dbus/META-INF/MANIFEST.MF # extensions/binding/pom.xml

htreu reviewed May 23, 2017

View reviewed changes

maggu2810 approved these changes May 23, 2017

View reviewed changes

sjsf force-pushed the fixSDF branch from 924fbbd to 2020b43 Compare May 23, 2017 15:15

maggu2810 approved these changes May 24, 2017

View reviewed changes

[Scheduler] Dropped usage of SimpleDateFormat for trace logging

29c59bb

...as it slows down the scheduler and was used in a dangeriously wrt thread-safety. fixes eclipse-archived#3505 fixes eclipse-archived#3506 Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>

sjsf force-pushed the fixSDF branch from 2020b43 to 29c59bb Compare May 24, 2017 08:36

amitjoy reviewed May 24, 2017

View reviewed changes

cweitkamp mentioned this pull request May 24, 2017

[avmfritz] Fixed NPE if temperature, powermeter or switch model is null openhab/openhab-addons#1887

Merged

amitjoy approved these changes May 24, 2017

View reviewed changes

maggu2810 merged commit caa91af into eclipse-archived:master May 26, 2017

kaikreuzer modified the milestone: 0.9.0 Jun 26, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Scheduler] Dropped usage of SimpleDateFormat for trace logging #3508

[Scheduler] Dropped usage of SimpleDateFormat for trace logging #3508

sjsf commented May 23, 2017

htreu May 23, 2017

sjsf May 23, 2017

maggu2810 May 23, 2017

maggu2810 left a comment

sjsf commented May 23, 2017

maggu2810 left a comment

maggu2810 May 24, 2017

maggu2810 May 24, 2017

sjsf May 24, 2017

maggu2810 May 24, 2017

maggu2810 May 24, 2017

amitjoy May 24, 2017

martinvw May 24, 2017

amitjoy May 24, 2017

martinvw May 24, 2017

		@@ -38,8 +37,6 @@

		private final static Logger logger = LoggerFactory.getLogger(ExpressionThreadPoolManager.class);

[Scheduler] Dropped usage of SimpleDateFormat for trace logging #3508

[Scheduler] Dropped usage of SimpleDateFormat for trace logging #3508

Conversation

sjsf commented May 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maggu2810 left a comment

Choose a reason for hiding this comment

sjsf commented May 23, 2017

maggu2810 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment