You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@felix.apache.org by GitBox <gi...@apache.org> on 2020/06/30 07:33:41 UTC
[GitHub] [felix-dev] amitjoy opened a new pull request #29: [FELIX-6265] Replaced Quartz with custom Cron Scheduler
amitjoy opened a new pull request #29:
URL: https://github.com/apache/felix-dev/pull/29
Fixed https://issues.apache.org/jira/browse/FELIX-6265
Signed-off-by: Amit Kumar Mondal <ad...@amitinside.com>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] amitjoy commented on a change in pull request #29: [FELIX-6265] Replaced Quartz with custom Cron Scheduler
Posted by GitBox <gi...@apache.org>.
amitjoy commented on a change in pull request #29:
URL: https://github.com/apache/felix-dev/pull/29#discussion_r447605674
##########
File path: healthcheck/core/src/main/java/org/apache/felix/hc/core/impl/executor/async/cron/CronSchedulerTask.java
##########
@@ -0,0 +1,53 @@
+package org.apache.felix.hc.core.impl.executor.async.cron;
+
+import org.slf4j.Logger;
Review comment:
Since this class is not leveraging SCR, I used slf4j
##########
File path: healthcheck/core/src/main/java/org/apache/felix/hc/core/impl/executor/async/cron/CronSchedulerTask.java
##########
@@ -0,0 +1,53 @@
+package org.apache.felix.hc.core.impl.executor.async.cron;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public final class CronSchedulerTask implements Runnable {
+
+ private final Logger logger = LoggerFactory.getLogger(getClass());
+
+ public final CronJob cronJob;
Review comment:
fixed
##########
File path: healthcheck/core/src/main/java/org/apache/felix/hc/core/impl/executor/async/cron/CronSchedulerTask.java
##########
@@ -0,0 +1,53 @@
+package org.apache.felix.hc.core.impl.executor.async.cron;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public final class CronSchedulerTask implements Runnable {
Review comment:
fixed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] amitjoy commented on a change in pull request #29: [FELIX-6265] Replaced Quartz with custom Cron Scheduler
Posted by GitBox <gi...@apache.org>.
amitjoy commented on a change in pull request #29:
URL: https://github.com/apache/felix-dev/pull/29#discussion_r447607678
##########
File path: healthcheck/core/src/main/java/org/apache/felix/hc/core/impl/executor/async/AsyncHealthCheckExecutor.java
##########
@@ -138,13 +135,7 @@ private boolean scheduleHealthCheck(HealthCheckMetadata descriptor) {
AsyncJob healthCheckAsyncJob = null;
if (isAsyncCron(descriptor)) {
-
- try {
- healthCheckAsyncJob = new AsyncQuartzCronJob(getAsyncJob(descriptor), quartzCronSchedulerProvider, "job-hc-" + descriptor.getServiceId(), "async-healthchecks", descriptor.getAsyncCronExpression());
- } catch(ClassNotFoundException|NoClassDefFoundError e) {
- LOG.warn("Can not schedule async health check '{}' with cron expression '{}' since quartz library is not on classpath", descriptor.getName(), descriptor.getAsyncCronExpression());
- return false;
- }
+ healthCheckAsyncJob = new AsyncCronJob(getAsyncJob(descriptor), healthCheckCronScheduler, "job-hc-" + descriptor.getServiceId(), descriptor.getAsyncCronExpression());
Review comment:
similarly here
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] JochenHiller commented on a change in pull request #29: [FELIX-6265] Replaced Quartz with custom Cron Scheduler
Posted by GitBox <gi...@apache.org>.
JochenHiller commented on a change in pull request #29:
URL: https://github.com/apache/felix-dev/pull/29#discussion_r447587525
##########
File path: healthcheck/core/src/main/java/org/apache/felix/hc/core/impl/executor/async/cron/CronScheduler.java
##########
@@ -0,0 +1,170 @@
+package org.apache.felix.hc.core.impl.executor.async.cron;
+
+import static java.util.concurrent.TimeUnit.SECONDS;
+
+import java.time.Duration;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Map.Entry;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.RejectedExecutionException;
+import java.util.concurrent.TimeUnit;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public final class CronScheduler implements AutoCloseable {
+
+ private final Logger logger = LoggerFactory.getLogger(getClass());
+
+ /**
+ * Object used for internal locking/notifications.
+ */
+ private final Object monitor = new Object();
+
+ /**
+ * List of managed tasks.
+ */
+ private final Map<String, CronSchedulerTask> tasks = new HashMap<>();
+
+ /**
+ * Task executor instance.
+ */
+ public final ExecutorService tasksExecutor;
+
+ /**
+ * Thread that makes all scheduling job.
+ */
+ public final SchedulerThread schedulerThread;
+
+ /**
+ * If scheduler is active or not.
+ */
+ private boolean isActive;
Review comment:
Should `isActive` be `volatile` ?
The access to it is not synchronized, so changes between `shutdown`, `isShutdown` and `run` might not be visible in other threads.
##########
File path: healthcheck/core/src/main/java/org/apache/felix/hc/core/impl/scheduling/AsyncCronJob.java
##########
@@ -0,0 +1,97 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The SF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations under the License.
+ */
+package org.apache.felix.hc.core.impl.scheduling;
+
+import org.apache.felix.hc.core.impl.executor.async.cron.CronJob;
+import org.apache.felix.hc.core.impl.executor.async.cron.CronScheduler;
+import org.apache.felix.hc.core.impl.executor.async.cron.HealthCheckCronScheduler;
+import org.slf4j.Logger;
Review comment:
Same here: SLF4J Logger vs. OSGi Logger?
##########
File path: healthcheck/core/src/main/java/org/apache/felix/hc/core/impl/executor/async/cron/CronScheduler.java
##########
@@ -0,0 +1,170 @@
+package org.apache.felix.hc.core.impl.executor.async.cron;
+
+import static java.util.concurrent.TimeUnit.SECONDS;
+
+import java.time.Duration;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Map.Entry;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.RejectedExecutionException;
+import java.util.concurrent.TimeUnit;
+
+import org.slf4j.Logger;
Review comment:
Is it intended to use SLF4j logger und not an OSGi logger?
##########
File path: healthcheck/core/src/main/java/org/apache/felix/hc/core/impl/executor/async/cron/CronSchedulerTask.java
##########
@@ -0,0 +1,53 @@
+package org.apache.felix.hc.core.impl.executor.async.cron;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public final class CronSchedulerTask implements Runnable {
+
+ private final Logger logger = LoggerFactory.getLogger(getClass());
+
+ public final CronJob cronJob;
+ public final CronScheduler scheduler;
+ public final CronParser sequenceGenerator;
+
+ volatile boolean executing;
+ volatile long lastExecutingTime = 0;
+ volatile long nextExecutingTime = 0;
+
+ public CronSchedulerTask(final CronScheduler scheduler, final CronJob cronJob,
+ final CronParser sequenceGenerator) {
+ this.scheduler = scheduler;
+ this.cronJob = cronJob;
+ this.sequenceGenerator = sequenceGenerator;
+ }
+
+ @Override
+ public void run() {
+ try {
+ cronJob.run();
Review comment:
Indent should be 2 spaces to left
##########
File path: healthcheck/core/src/main/java/org/apache/felix/hc/core/impl/executor/async/AsyncHealthCheckExecutor.java
##########
@@ -138,13 +135,7 @@ private boolean scheduleHealthCheck(HealthCheckMetadata descriptor) {
AsyncJob healthCheckAsyncJob = null;
if (isAsyncCron(descriptor)) {
-
- try {
- healthCheckAsyncJob = new AsyncQuartzCronJob(getAsyncJob(descriptor), quartzCronSchedulerProvider, "job-hc-" + descriptor.getServiceId(), "async-healthchecks", descriptor.getAsyncCronExpression());
- } catch(ClassNotFoundException|NoClassDefFoundError e) {
- LOG.warn("Can not schedule async health check '{}' with cron expression '{}' since quartz library is not on classpath", descriptor.getName(), descriptor.getAsyncCronExpression());
- return false;
- }
+ healthCheckAsyncJob = new AsyncCronJob(getAsyncJob(descriptor), healthCheckCronScheduler, "job-hc-" + descriptor.getServiceId(), descriptor.getAsyncCronExpression());
Review comment:
Should be indented 2 spaces to right
##########
File path: healthcheck/core/src/main/java/org/apache/felix/hc/core/impl/scheduling/AsyncCronJob.java
##########
@@ -0,0 +1,97 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The SF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations under the License.
+ */
+package org.apache.felix.hc.core.impl.scheduling;
+
+import org.apache.felix.hc.core.impl.executor.async.cron.CronJob;
+import org.apache.felix.hc.core.impl.executor.async.cron.CronScheduler;
+import org.apache.felix.hc.core.impl.executor.async.cron.HealthCheckCronScheduler;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Runs health checks that are configured with a cron expression for asynchronous execution.
+ */
+public class AsyncCronJob extends AsyncJob {
+ private final Logger logger = LoggerFactory.getLogger(getClass());
+
+ private final CronJob cronJob;
+ protected final HealthCheckCronScheduler healtchCheckScheduler;
+
+ public AsyncCronJob(Runnable runnable, HealthCheckCronScheduler healtchCheckScheduler, String id, String cronExpression) throws ClassNotFoundException {
+ super(runnable);
+ this.healtchCheckScheduler = healtchCheckScheduler;
+ this.cronJob = new HealthCheckJob(cronExpression, id);
+ }
+
+ public boolean schedule() {
+ try {
+ CronScheduler scheduler = healtchCheckScheduler.getScheduler();
+ scheduler.schedule(cronJob);
+ logger.info("Scheduled job {} with trigger {}", cronJob.name(), cronJob.cron());
+ return true;
+ } catch (Exception e) {
+ logger.error("Could not schedule job for " + runnable + ": " + e, e);
+ return false;
+ }
+
Review comment:
Remove empty line
##########
File path: healthcheck/core/src/test/java/org/apache/felix/hc/core/impl/executor/async/cron/CronSchedulerTest.java
##########
@@ -0,0 +1,114 @@
+package org.apache.felix.hc.core.impl.executor.async.cron;
+
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
+
+import org.apache.felix.hc.core.impl.executor.async.cron.SampleCronJob1.Callback;
+import org.junit.Assert;
+import org.junit.Test;
+
+public class CronSchedulerTest extends Assert {
+
+ private static final Object SYNC_OBJECT = new Object();
+
+ @Test
+ public void checkSchedulerCheckSingleThread() throws InterruptedException {
+ try (final CronScheduler scheduler = new CronScheduler(Executors.newFixedThreadPool(1), 0, 1, TimeUnit.SECONDS, "T1")) {
+ final int nIterations = 3;
+
+ final SampleCronJob1 service = new SampleCronJob2(s -> {
+ System.out.println("Iteration: " + s.counter);
Review comment:
Is it OK to write to console during tests in Apache Felix project?
##########
File path: healthcheck/core/src/main/java/org/apache/felix/hc/core/impl/executor/async/cron/CronSchedulerTask.java
##########
@@ -0,0 +1,53 @@
+package org.apache.felix.hc.core.impl.executor.async.cron;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public final class CronSchedulerTask implements Runnable {
+
+ private final Logger logger = LoggerFactory.getLogger(getClass());
+
+ public final CronJob cronJob;
Review comment:
Why are these 3 variables public? Would `package` scope not be enough?
##########
File path: healthcheck/core/src/main/java/org/apache/felix/hc/core/impl/scheduling/AsyncCronJob.java
##########
@@ -0,0 +1,97 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The SF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations under the License.
+ */
+package org.apache.felix.hc.core.impl.scheduling;
+
+import org.apache.felix.hc.core.impl.executor.async.cron.CronJob;
+import org.apache.felix.hc.core.impl.executor.async.cron.CronScheduler;
+import org.apache.felix.hc.core.impl.executor.async.cron.HealthCheckCronScheduler;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Runs health checks that are configured with a cron expression for asynchronous execution.
+ */
+public class AsyncCronJob extends AsyncJob {
+ private final Logger logger = LoggerFactory.getLogger(getClass());
+
+ private final CronJob cronJob;
+ protected final HealthCheckCronScheduler healtchCheckScheduler;
+
+ public AsyncCronJob(Runnable runnable, HealthCheckCronScheduler healtchCheckScheduler, String id, String cronExpression) throws ClassNotFoundException {
+ super(runnable);
+ this.healtchCheckScheduler = healtchCheckScheduler;
+ this.cronJob = new HealthCheckJob(cronExpression, id);
+ }
+
+ public boolean schedule() {
+ try {
+ CronScheduler scheduler = healtchCheckScheduler.getScheduler();
+ scheduler.schedule(cronJob);
+ logger.info("Scheduled job {} with trigger {}", cronJob.name(), cronJob.cron());
+ return true;
+ } catch (Exception e) {
+ logger.error("Could not schedule job for " + runnable + ": " + e, e);
+ return false;
+ }
+
+ }
+
+ @Override
+ public boolean unschedule() {
+ CronScheduler scheduler = healtchCheckScheduler.getScheduler();
+ String name = cronJob.name();
+ logger.debug("Unscheduling job {}", name);
+ try {
+ scheduler.remove(name);
+ return true;
+ } catch (Exception e) {
+ logger.error("Could not unschedule job for " + name + ": " + e, e);
+ return false;
+ }
+ }
+
+ @Override
+ public String toString() {
+ return "[Async cron job for " + runnable + "]";
+ }
+
+ private class HealthCheckJob implements CronJob {
+
+ private String name;
+ private String cronExpression;
+
+ public HealthCheckJob(String cronExpression, String name) {
+ this.name = name;
+ this.cronExpression = cronExpression;
+ }
+
+ @Override
Review comment:
Indent to same column like method
##########
File path: healthcheck/core/src/main/java/org/apache/felix/hc/core/impl/executor/async/cron/CronSchedulerTask.java
##########
@@ -0,0 +1,53 @@
+package org.apache.felix.hc.core.impl.executor.async.cron;
+
+import org.slf4j.Logger;
Review comment:
Same here: SLF4J Logger vs. OSGi Logger?
##########
File path: healthcheck/core/src/main/java/org/apache/felix/hc/core/impl/executor/async/cron/CronSchedulerTask.java
##########
@@ -0,0 +1,53 @@
+package org.apache.felix.hc.core.impl.executor.async.cron;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public final class CronSchedulerTask implements Runnable {
Review comment:
Could be the whole class here be in `package` scope?
##########
File path: healthcheck/core/src/main/java/org/apache/felix/hc/core/impl/scheduling/AsyncCronJob.java
##########
@@ -0,0 +1,97 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The SF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations under the License.
+ */
+package org.apache.felix.hc.core.impl.scheduling;
+
+import org.apache.felix.hc.core.impl.executor.async.cron.CronJob;
+import org.apache.felix.hc.core.impl.executor.async.cron.CronScheduler;
+import org.apache.felix.hc.core.impl.executor.async.cron.HealthCheckCronScheduler;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Runs health checks that are configured with a cron expression for asynchronous execution.
+ */
+public class AsyncCronJob extends AsyncJob {
+ private final Logger logger = LoggerFactory.getLogger(getClass());
Review comment:
Indent 2 spaces to left
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] amitjoy commented on pull request #29: [FELIX-6265] Replaced Quartz with custom Cron Scheduler
Posted by GitBox <gi...@apache.org>.
amitjoy commented on pull request #29:
URL: https://github.com/apache/felix-dev/pull/29#issuecomment-652387631
@ghenzler Don't worry about it. I have touched as little as possible and maintained a clean separation of packages for better comprehension. Thanks a lot again for your quick assistance.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] amitjoy commented on a change in pull request #29: [FELIX-6265] Replaced Quartz with custom Cron Scheduler
Posted by GitBox <gi...@apache.org>.
amitjoy commented on a change in pull request #29:
URL: https://github.com/apache/felix-dev/pull/29#discussion_r447607309
##########
File path: healthcheck/core/src/main/java/org/apache/felix/hc/core/impl/scheduling/AsyncCronJob.java
##########
@@ -0,0 +1,97 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The SF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations under the License.
+ */
+package org.apache.felix.hc.core.impl.scheduling;
+
+import org.apache.felix.hc.core.impl.executor.async.cron.CronJob;
+import org.apache.felix.hc.core.impl.executor.async.cron.CronScheduler;
+import org.apache.felix.hc.core.impl.executor.async.cron.HealthCheckCronScheduler;
+import org.slf4j.Logger;
Review comment:
Since this class is not leveraging SCR, I used slf4j instead.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] amitjoy commented on a change in pull request #29: [FELIX-6265] Replaced Quartz with custom Cron Scheduler
Posted by GitBox <gi...@apache.org>.
amitjoy commented on a change in pull request #29:
URL: https://github.com/apache/felix-dev/pull/29#discussion_r447604922
##########
File path: healthcheck/core/src/main/java/org/apache/felix/hc/core/impl/executor/async/AsyncHealthCheckExecutor.java
##########
@@ -138,13 +135,7 @@ private boolean scheduleHealthCheck(HealthCheckMetadata descriptor) {
AsyncJob healthCheckAsyncJob = null;
if (isAsyncCron(descriptor)) {
-
- try {
- healthCheckAsyncJob = new AsyncQuartzCronJob(getAsyncJob(descriptor), quartzCronSchedulerProvider, "job-hc-" + descriptor.getServiceId(), "async-healthchecks", descriptor.getAsyncCronExpression());
- } catch(ClassNotFoundException|NoClassDefFoundError e) {
- LOG.warn("Can not schedule async health check '{}' with cron expression '{}' since quartz library is not on classpath", descriptor.getName(), descriptor.getAsyncCronExpression());
- return false;
- }
+ healthCheckAsyncJob = new AsyncCronJob(getAsyncJob(descriptor), healthCheckCronScheduler, "job-hc-" + descriptor.getServiceId(), descriptor.getAsyncCronExpression());
Review comment:
There was a long discussion earlier to use specific formatting standard for Felix but as far as I got to know, it is not decided and that's why I wrote everything without any formatting applied. In Eclipse, the changes look good but due to formatting standard in GitHub, it looks different.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] amitjoy commented on a change in pull request #29: [FELIX-6265] Replaced Quartz with custom Cron Scheduler
Posted by GitBox <gi...@apache.org>.
amitjoy commented on a change in pull request #29:
URL: https://github.com/apache/felix-dev/pull/29#discussion_r447605392
##########
File path: healthcheck/core/src/main/java/org/apache/felix/hc/core/impl/executor/async/cron/CronScheduler.java
##########
@@ -0,0 +1,170 @@
+package org.apache.felix.hc.core.impl.executor.async.cron;
+
+import static java.util.concurrent.TimeUnit.SECONDS;
+
+import java.time.Duration;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Map.Entry;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.RejectedExecutionException;
+import java.util.concurrent.TimeUnit;
+
+import org.slf4j.Logger;
Review comment:
Since this class is not leveraging SCR, I used slf4j and slf4j is also used in many other places in Felix HC.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] amitjoy commented on pull request #29: [FELIX-6265] Replaced Quartz with custom Cron Scheduler
Posted by GitBox <gi...@apache.org>.
amitjoy commented on pull request #29:
URL: https://github.com/apache/felix-dev/pull/29#issuecomment-652272080
@ghenzler Thanks a lot for your valuable feedback. I agree with you completely that it makes sense to support cron with and without the availability of Quartz. It will enable consumers to decide whether to use Quartz or the tiny cron scheduler that comes with it. Yeah, you are right that it is quite a bit of changes but I would love to amend the necessary changes. I will keep you updated on the progress.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] ghenzler edited a comment on pull request #29: [FELIX-6265] Replaced Quartz with custom Cron Scheduler
Posted by GitBox <gi...@apache.org>.
ghenzler edited a comment on pull request #29:
URL: https://github.com/apache/felix-dev/pull/29#issuecomment-652386454
@amitjoy thanks for taking a look, I just created 8f78a180b24740a2c871 to make it easier for you because when looking into it I found the current code is a bit confusing to extend. So with 8f78a180b24740a2c871 you should be just able to extend on AsyncSimpleCronJob without touching much else... the cron specific code can go in package `org.apache.felix.hc.core.impl.scheduling.cron` that should be created. I also renamed FELIX-6265 a bit to make it clearer that this is not replacing quartz (but just a fallback for the case if quartz is not there).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] amitjoy commented on a change in pull request #29: [FELIX-6265] Replaced Quartz with custom Cron Scheduler
Posted by GitBox <gi...@apache.org>.
amitjoy commented on a change in pull request #29:
URL: https://github.com/apache/felix-dev/pull/29#discussion_r447607038
##########
File path: healthcheck/core/src/main/java/org/apache/felix/hc/core/impl/scheduling/AsyncCronJob.java
##########
@@ -0,0 +1,97 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The SF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations under the License.
+ */
+package org.apache.felix.hc.core.impl.scheduling;
+
+import org.apache.felix.hc.core.impl.executor.async.cron.CronJob;
+import org.apache.felix.hc.core.impl.executor.async.cron.CronScheduler;
+import org.apache.felix.hc.core.impl.executor.async.cron.HealthCheckCronScheduler;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Runs health checks that are configured with a cron expression for asynchronous execution.
+ */
+public class AsyncCronJob extends AsyncJob {
+ private final Logger logger = LoggerFactory.getLogger(getClass());
+
+ private final CronJob cronJob;
+ protected final HealthCheckCronScheduler healtchCheckScheduler;
+
+ public AsyncCronJob(Runnable runnable, HealthCheckCronScheduler healtchCheckScheduler, String id, String cronExpression) throws ClassNotFoundException {
+ super(runnable);
+ this.healtchCheckScheduler = healtchCheckScheduler;
+ this.cronJob = new HealthCheckJob(cronExpression, id);
+ }
+
+ public boolean schedule() {
+ try {
+ CronScheduler scheduler = healtchCheckScheduler.getScheduler();
+ scheduler.schedule(cronJob);
+ logger.info("Scheduled job {} with trigger {}", cronJob.name(), cronJob.cron());
+ return true;
+ } catch (Exception e) {
+ logger.error("Could not schedule job for " + runnable + ": " + e, e);
+ return false;
+ }
+
+ }
+
+ @Override
+ public boolean unschedule() {
+ CronScheduler scheduler = healtchCheckScheduler.getScheduler();
+ String name = cronJob.name();
+ logger.debug("Unscheduling job {}", name);
+ try {
+ scheduler.remove(name);
+ return true;
+ } catch (Exception e) {
+ logger.error("Could not unschedule job for " + name + ": " + e, e);
+ return false;
+ }
+ }
+
+ @Override
+ public String toString() {
+ return "[Async cron job for " + runnable + "]";
+ }
+
+ private class HealthCheckJob implements CronJob {
+
+ private String name;
+ private String cronExpression;
+
+ public HealthCheckJob(String cronExpression, String name) {
+ this.name = name;
+ this.cronExpression = cronExpression;
+ }
+
+ @Override
Review comment:
In Eclipse, this looks good but due to formatting standard in GitHub, it looks different here.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] ghenzler commented on pull request #29: [FELIX-6265] Introduced Embedded Cron Scheduler
Posted by GitBox <gi...@apache.org>.
ghenzler commented on pull request #29:
URL: https://github.com/apache/felix-dev/pull/29#issuecomment-653366854
Hi @amitjoy thanks for the contribution. Based on your PR I made the following improvements in da6061d2d76b5926b77b82:
* I remove a lot of boilerplate classes (to keep it as minimal as possible this will be good for IoT)
* I introduced support for 7th field year for at least `*`
** `*` could be easily supported because it is the same as omitting it, I had an existing expression that was failing
** I removed the shortcut `@reboot` (because for that we would need proper year field support, PR welcome)
* The class `EmbeddedCronParserTest` was using the deprecated Date constructor, fixed that and moved all tests for `EmbeddedCronParser` into `EmbeddedCronParserTest`
* The class `EmbeddedCronScheduler` is a lot simpler now and just uses the JDK `ScheduledExecutorService` for without complicated wait/notify mechanics.
Everything is together on master now, please test with your setup if everything works as expected.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] amitjoy commented on a change in pull request #29: [FELIX-6265] Replaced Quartz with custom Cron Scheduler
Posted by GitBox <gi...@apache.org>.
amitjoy commented on a change in pull request #29:
URL: https://github.com/apache/felix-dev/pull/29#discussion_r447606804
##########
File path: healthcheck/core/src/test/java/org/apache/felix/hc/core/impl/executor/async/cron/CronSchedulerTest.java
##########
@@ -0,0 +1,114 @@
+package org.apache.felix.hc.core.impl.executor.async.cron;
+
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
+
+import org.apache.felix.hc.core.impl.executor.async.cron.SampleCronJob1.Callback;
+import org.junit.Assert;
+import org.junit.Test;
+
+public class CronSchedulerTest extends Assert {
+
+ private static final Object SYNC_OBJECT = new Object();
+
+ @Test
+ public void checkSchedulerCheckSingleThread() throws InterruptedException {
+ try (final CronScheduler scheduler = new CronScheduler(Executors.newFixedThreadPool(1), 0, 1, TimeUnit.SECONDS, "T1")) {
+ final int nIterations = 3;
+
+ final SampleCronJob1 service = new SampleCronJob2(s -> {
+ System.out.println("Iteration: " + s.counter);
Review comment:
fixed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] amitjoy commented on pull request #29: [FELIX-6265] Introduced Embedded Cron Scheduler
Posted by GitBox <gi...@apache.org>.
amitjoy commented on pull request #29:
URL: https://github.com/apache/felix-dev/pull/29#issuecomment-652443095
Just for the info: I have followed the proposal by @ghenzler to use Quartz optionally if Quartz packaged is wired to the bundle and if not, we will fallback to the usage of a tiny embedded cron scheduler. This will enable Felix HC to be used in small footprint IoT edge devices as well.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] amitjoy commented on a change in pull request #29: [FELIX-6265] Replaced Quartz with custom Cron Scheduler
Posted by GitBox <gi...@apache.org>.
amitjoy commented on a change in pull request #29:
URL: https://github.com/apache/felix-dev/pull/29#discussion_r447605485
##########
File path: healthcheck/core/src/main/java/org/apache/felix/hc/core/impl/executor/async/cron/CronScheduler.java
##########
@@ -0,0 +1,170 @@
+package org.apache.felix.hc.core.impl.executor.async.cron;
+
+import static java.util.concurrent.TimeUnit.SECONDS;
+
+import java.time.Duration;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Map.Entry;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.RejectedExecutionException;
+import java.util.concurrent.TimeUnit;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public final class CronScheduler implements AutoCloseable {
+
+ private final Logger logger = LoggerFactory.getLogger(getClass());
+
+ /**
+ * Object used for internal locking/notifications.
+ */
+ private final Object monitor = new Object();
+
+ /**
+ * List of managed tasks.
+ */
+ private final Map<String, CronSchedulerTask> tasks = new HashMap<>();
+
+ /**
+ * Task executor instance.
+ */
+ public final ExecutorService tasksExecutor;
+
+ /**
+ * Thread that makes all scheduling job.
+ */
+ public final SchedulerThread schedulerThread;
+
+ /**
+ * If scheduler is active or not.
+ */
+ private boolean isActive;
Review comment:
Fixed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] amitjoy commented on a change in pull request #29: [FELIX-6265] Replaced Quartz with custom Cron Scheduler
Posted by GitBox <gi...@apache.org>.
amitjoy commented on a change in pull request #29:
URL: https://github.com/apache/felix-dev/pull/29#discussion_r447606629
##########
File path: healthcheck/core/src/main/java/org/apache/felix/hc/core/impl/executor/async/cron/CronSchedulerTask.java
##########
@@ -0,0 +1,53 @@
+package org.apache.felix.hc.core.impl.executor.async.cron;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public final class CronSchedulerTask implements Runnable {
+
+ private final Logger logger = LoggerFactory.getLogger(getClass());
+
+ public final CronJob cronJob;
+ public final CronScheduler scheduler;
+ public final CronParser sequenceGenerator;
+
+ volatile boolean executing;
+ volatile long lastExecutingTime = 0;
+ volatile long nextExecutingTime = 0;
+
+ public CronSchedulerTask(final CronScheduler scheduler, final CronJob cronJob,
+ final CronParser sequenceGenerator) {
+ this.scheduler = scheduler;
+ this.cronJob = cronJob;
+ this.sequenceGenerator = sequenceGenerator;
+ }
+
+ @Override
+ public void run() {
+ try {
+ cronJob.run();
Review comment:
In Eclipse, this looks good but due to formatting standard in GitHub, it looks different here.
##########
File path: healthcheck/core/src/main/java/org/apache/felix/hc/core/impl/scheduling/AsyncCronJob.java
##########
@@ -0,0 +1,97 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The SF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations under the License.
+ */
+package org.apache.felix.hc.core.impl.scheduling;
+
+import org.apache.felix.hc.core.impl.executor.async.cron.CronJob;
+import org.apache.felix.hc.core.impl.executor.async.cron.CronScheduler;
+import org.apache.felix.hc.core.impl.executor.async.cron.HealthCheckCronScheduler;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Runs health checks that are configured with a cron expression for asynchronous execution.
+ */
+public class AsyncCronJob extends AsyncJob {
+ private final Logger logger = LoggerFactory.getLogger(getClass());
+
+ private final CronJob cronJob;
+ protected final HealthCheckCronScheduler healtchCheckScheduler;
+
+ public AsyncCronJob(Runnable runnable, HealthCheckCronScheduler healtchCheckScheduler, String id, String cronExpression) throws ClassNotFoundException {
+ super(runnable);
+ this.healtchCheckScheduler = healtchCheckScheduler;
+ this.cronJob = new HealthCheckJob(cronExpression, id);
+ }
+
+ public boolean schedule() {
+ try {
+ CronScheduler scheduler = healtchCheckScheduler.getScheduler();
+ scheduler.schedule(cronJob);
+ logger.info("Scheduled job {} with trigger {}", cronJob.name(), cronJob.cron());
+ return true;
+ } catch (Exception e) {
+ logger.error("Could not schedule job for " + runnable + ": " + e, e);
+ return false;
+ }
+
Review comment:
fixed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] amitjoy commented on a change in pull request #29: [FELIX-6265] Replaced Quartz with custom Cron Scheduler
Posted by GitBox <gi...@apache.org>.
amitjoy commented on a change in pull request #29:
URL: https://github.com/apache/felix-dev/pull/29#discussion_r447606331
##########
File path: healthcheck/core/src/main/java/org/apache/felix/hc/core/impl/scheduling/AsyncCronJob.java
##########
@@ -0,0 +1,97 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The SF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations under the License.
+ */
+package org.apache.felix.hc.core.impl.scheduling;
+
+import org.apache.felix.hc.core.impl.executor.async.cron.CronJob;
+import org.apache.felix.hc.core.impl.executor.async.cron.CronScheduler;
+import org.apache.felix.hc.core.impl.executor.async.cron.HealthCheckCronScheduler;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Runs health checks that are configured with a cron expression for asynchronous execution.
+ */
+public class AsyncCronJob extends AsyncJob {
+ private final Logger logger = LoggerFactory.getLogger(getClass());
Review comment:
Since we don't have any proper formatting standard for Felix, I write everything without any formatting applied.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] ghenzler merged pull request #29: [FELIX-6265] Introduced Embedded Cron Scheduler
Posted by GitBox <gi...@apache.org>.
ghenzler merged pull request #29:
URL: https://github.com/apache/felix-dev/pull/29
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] ghenzler commented on pull request #29: [FELIX-6265] Replaced Quartz with custom Cron Scheduler
Posted by GitBox <gi...@apache.org>.
ghenzler commented on pull request #29:
URL: https://github.com/apache/felix-dev/pull/29#issuecomment-652386454
@amitjoy thanks for taking a look, I just created 8f78a180b24740a2c871 to make it easier for you because when looking into it I found the current code is a bit confusing to extend. So with 8f78a180b24740a2c871 you should be just able to extend on AsyncSimpleCronJob without touching much else... the cron specific code can go in package `org.apache.felix.hc.core.impl.scheduling.cron` that should be created.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] amitjoy commented on pull request #29: [FELIX-6265] Introduced Embedded Cron Scheduler
Posted by GitBox <gi...@apache.org>.
amitjoy commented on pull request #29:
URL: https://github.com/apache/felix-dev/pull/29#issuecomment-652441279
@ghenzler @JochenHiller I have updated the MR. You can now have a look.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org