Initial contribution of the bespoke scheduler infrastructure for ESH V2.0 #959
Conversation
|
Stefan, Kai, this should be ok for a first review round. |
| */ | ||
| public abstract class AbstractExpression<E extends AbstractExpressionPart> implements Expression { | ||
|
|
||
| final Logger logger = LoggerFactory.getLogger(this.getClass().getName()); |
|
From a legal point of view, that should be fine now as far as I can see. There are a few minor remarks that I left in the code, I hope they make sense to you? Generally I about the naming, the class names clearly make sense, but e.g. "Expression" is quite generic and it's currently located in the equally generic .core.common package - therefore some namespacing would be good, e.g. by using a dedicated sub-package. Functionality wise - it's hard to tell in theory. The logic you have implemented is a perfect target for unit-tests. And I don't feel comfortable with having such logic without any tests at all. Would you happen to have some tests for your implementation too? I guess you must have somehow tried it out as well... Regarding the PR, I think it would make sense to squash the commits in order to not have the first implementation with the IP issues merged. |
|
@SJKA I have never written groovy stuff in my life, so I am totally lost with respect to the unit testing. However, that being said, i have tested a ton of expressions for both cron and rfc5545 "manually". These were expressions I copied from websites that also contained the supposed output, and I compared to that to validate the correctness of the code. Expression we could change to SchedulerExpression or SchedulingExpression? |
|
@SJKA, @kaikreuzer any next steps? This PR is necessary to be able to complete the Automation PR, which in turn is necessary for the CalDav stuff that is nearly finished |
I agree - actually, we currently even have a separate bundle (.core.scheduler); if we argue that it is a central part that everybody will need and we want to reduce the number of bundles, I am also fine with putting the code in the .core bundle - but then it should be in the package .core.scheduler and not .core.common.
Note that tests do not have to be written in Groovy - pure Java is fine as well. @maggu2810 As this is a major code contribution, I would love to have your input and comments on this PR as well! |
How do I see or test that the @test is working? Can I do that from within the Eclipse IDE? K |
|
Yes, simply right-click the test class and choose "Run As -> JUnit Test". |
as it is, .core.scheduler could/should disappear no? as it only links in the Quartz libs. FYI, I use it throughout the other stuff I mentioned (migration of DSL Script/Rule to Automation) as well as CalDav stuff, so I would expect it to be .core. e.g. a renewed .core.scheduler then |
|
Yes, I am fine with that. So .core bundle and .core.scheduler package it is. |
Sure, it is already on my Todo list. |
| */ | ||
| public final class CronExpression extends AbstractExpression<CronExpressionPart> { | ||
|
|
||
| private final static Logger logger = LoggerFactory.getLogger(CronExpression.class); |
There was a problem hiding this comment.
We are using non-static logger instances.
|
This PR is a big one and to review it I want to use the code base on my machine (IDE etc.). |
|
@maggu2810 need to check that, as it works at my side (in the IDE). That being said, could maybe due to timezone differences, and in that case something else needs fixing. |
| DateFormat df3 = new SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ssXXX"); | ||
| df3.setTimeZone(getTimeZone()); | ||
| theDate = df3.parse(getExpression()); | ||
| } catch (Exception g) { |
There was a problem hiding this comment.
Instead of the encapsulated expressions, I would prefer to use another function.
What do you think about e.g. this one:
/**
* Try to parse using a set of valid formats.
*
* @param formats the date format strings in the order they should be used
* @return true if parsing succeeded by a format, otherwise false
*/
private boolean parseFormats(final String[] formats) {
for (final String format : formats) {
try {
final DateFormat df = new SimpleDateFormat(format);
df.setTimeZone(getTimeZone());
theDate = df.parse(getExpression());
return true;
} catch (final Exception e) {
// Try next one...
}
}
return false;
}
@Override
public void parse() throws ParseException {
// Unfortunately, the time zone formats available to SimpleDateFormat (Java 6 and earlier) are not ISO 8601
// compliant. SimpleDateFormat understands time zone strings like "GMT+01:00" or "+0100", the latter
// according to RFC # 822.
//
// Even if Java 7 added support for time zone descriptors according to ISO 8601, SimpleDateFormat is still
// not able to properly parse a complete date string, as it has no support for optional parts.
//
// Reformatting the input string using regexp is certainly one possibility, but the replacement rules are
// not as simple
//
// Some time zones are not full hours off UTC, so the string does not necessarily end with ":00".
// ISO8601 allows only the number of hours to be included in the time zone, so "+01" is equivalent to
// "+01:00". ISO8601 allows the usage of "Z" to indicate UTC instead of "+00:00".
// The easier solution is to use the data type converter in JAXB, since JAXB must be able to parse
// ISO8601 date string according to the XML Schema specification.
// try {
// Calendar cal = DatatypeConverter.parseDateTime(getExpression());
// theDate = cal.getTime();
// } catch (Exception e) {
// throw new ParseException(getPart() + " is not an ISO8601 formatted date", 0);
// }
if (!parseFormats(
new String[] { "yyyy-MM-dd'T'HH:mm:ssX", "yyyy-MM-dd'T'HH:mm:ssXX", "yyyy-MM-dd'T'HH:mm:ssXXX" })) {
throw new ParseException(getPart() + " is not an ISO8601 formatted date", 0);
}
}
|
I have only a short look over the files "CronExpression" and "RecurrenceExpression". The logic is / should be tested using unit tests. The unit tests will identify failures, etc. |
|
@kaikreuzer @SJKA @maggu2810 I have processed all comments that we agreed upon |
| throw new NullPointerException(); | ||
| } | ||
|
|
||
| this.setCorePoolSize(this.getCorePoolSize() + 1); |
There was a problem hiding this comment.
This looks fundamentally flawed to me - when handing over a job for scheduling, you have to increase the pool size (btw, is it ever decreased again?) and you block the thread by a Thread.sleep until the time for execution is reached?
This is imho not at all the idea of a scheduler service. There should be a single thread responsible for tracking when which job needs to be executed and it must not make use of the thread pool itself for this.
Compare with ScheduledThreadPoolExecutor, which uses a delayedExecute() method, which puts the tasks into the internal DelayedWorkQueue for distributing it to the thread pool, when it is time to do so.
There was a problem hiding this comment.
You are too quick here I think. Expressions can have multiple execution moments, so, in order to keep it simple there is one thread managing each expression, e.g. the scheduleTask.Putting the thread to sleep will not hurt the system either I think. IMHO things will get complicated if you want to put that in a single thread. Personally, I would refrain from interfering with the inner bodyworks of ScheduledThreadPoolExecutor (delayedExecute is private). If you put it in 1 thread, that thread would have to run like crazy in order to constantly evaluate the expression execution moments, no? In the best case you would have to evaluate all getTimeAfter(now) of all Expressions, and put it to sleep to the smallest time that comes up; and then you have to manage that thread every time a new expression is schedule()ed.
On the pool size you might be right, but I don't remember if ScheduledThreadPoolExecutor automatically reduces that after a while.
There was a problem hiding this comment.
Expressions can have multiple execution moments
The same is true for the scheduleAtFixedRate jobs in the ScheduledThreadPoolExecutor.
one thread managing each expression
This is exactly the no-go that I mean. This completely defeats the idea of a thread pool.
Putting the thread to sleep will not hurt the system either I think
It does. Every single thread is expensive and the thread pools are there to reduce their number.
would have to run like crazy in order to constantly evaluate the expression execution moments, no?
No. Because you could only have to check your queue (which can be sorted by next execution time), if there is a job at the top of the queue that needs to be executed. If not, just sleep for until its startTime (or until a new job is added to the queue, which might be the top most job then).
There was a problem hiding this comment.
Q: What would be the concern of having the "managing thread" in the thread pool itself?
Putting in in the pool itself would make it easier to track it. If not, we are destined to use thread signalling to wake up the sleeping management thread whenever the queue order changes etc.
There was a problem hiding this comment.
No general concern, but it does not really make much sense, since this thread will be blocked eternally and thus be effectively not be part of the pool anyhow, right? I am not sure if I understand your argument for using a thread in the pool. I think having a dedicated thread makes it easier to give it access to the queue (the ones from the pool should not know about the queue) and also to give it an explicit name, so that it is easily identified in the debugger.
|
I tried it with a few examples and it seems to work ;-) A few questions remain: 1. what if we have a large volume of expressions to evaluate. 2. what if we want to reuse the same task with different expressions, knowing that the "task" is the pivotal/unique object that makes the underlying scheduler tick, e.g. should we foresee the possibility to have the same task object being scheduled with different expressions or not? |
We should probably keep a sorted Map of tasks to not having to iterate through the whole list again and again. Not sure how the normal scheduler handles this, maybe it is worth to use it as a source for ideas.
What problem is there, if a user passes the same instance with two different expressions to the scheduler? Does Quartz support this? |
| @@ -5,16 +5,25 @@ | |||
| * which accompanies this distribution, and is available at | |||
| * http://www.eclipse.org/legal/epl-v10.html | |||
| */ | |||
| package org.eclipse.smarthome.core.common; | |||
| package org.eclipse.smarthome.core.scheduler; | |||
There was a problem hiding this comment.
This class should imho stay where it was, so that we keep backward compatibility.
So we could either
a) have a dependency from core.common to core.scheduler by this
b) have getExpressionScheduledPool() return a ThreadPoolExecutor that needs to be casted by the caller
c) Introduce a core.scheduler.ExpressionThreadPoolManager class that extends the normal one.
No solution looks ideal to me, so discussion is opened :-)
There was a problem hiding this comment.
I am open for anything basically. I will only have 1 problem in the coming week(s) and that is a lack of time, so I am open for you/someone else to take over the PR to get it finished.
There was a problem hiding this comment.
I think option b is probably the quickest intermediate solution then. So could you move the class back and squash all your commits? I can then try to take over further processing (probably soon creating a CQ as this might also take a while to be approved). Thanks!
From the Quartz doc: You can create a single job class, and store many 'instance definitions' of it within the scheduler by creating multiple instances of JobDetails - each with its own set of properties and JobDataMap - and adding them all to the scheduler. When you schedule a job in Quartz, you have the option to replace any existing job/trigger. I am not sure we have to replicate the whole quartz framework here. I was just wondering if we need/have this "job reuse" requirements in ESH |
|
Can we finish this PR as other work depends on it? |
|
It looks as if I didn't express myself right in #959 (comment) and you have now moved ALL classes from scheduler to common. In my suggestion, I was ONLY talking about the class ThreadPoolExecutor that needs to stay in common in order not to break any backward compatibility. All the rest is supposed to be in scheduler as already discussed before. |
|
@maggu2810 Are you fine with this PR in general? I would then create the CQ once @kgoderis has moved the classes back to the scheduler package. |
|
If I look at the "Files changed" view there is still commented stuff, that needs to be changed (IMHO). |
|
@kaikreuzer heu? I reverted the commit and squashed the whole lot. Dunno understand. In order to avoid further confusion, I suggest you take over and put it they way you think is best..... |
All I asked for was that "This class (ThreadPoolManager) should imho stay where it was".
Ok, I'll see when I find the time... |
Signed-off-by: Karel Goderis <karel.goderis@me.com>
|
@kaikreuzer cleaned up. Only the Quartz related questions are still to be discussed. |
|
I still do not see the core.scheduler package being back. Never mind, I will try to find the time to do it myself soon... |
|
I reverted it back to the original situation before all the back/fwd movements |
Signed-off-by: Kai Kreuzer <kai@openhab.org>
|
Ok, so I have created kgoderis#4, which you can merge and squash - once done, I will create the CQ. |
moved new scheduler stuff to core.scheduler package
|
Opened a new clean PR |
|
Superseded by #1239 |
Signed-off-by: Karel Goderis karel.goderis@me.com