OHFJIRA-27 - Add support for clustered jobs (quartz) by openhubframework · Pull Request #33 · OpenWiseSolutions/openhub-framework

openhubframework · 2017-03-12T19:01:44Z

In cluster we need two types of job:

jobs which can run on random node and can be executed parallel (it's not condition to start jobs in the same time)
jobs which can run only once in the cluster, doesn't matter on which node

Solution:

remove apache camel quartz component
jobs and triggers is defined with annotation QuartzJob (use only on method that will be invoked), QuartzSimpleTrigger and QuartzCronTrigger
in annotation QuartzJob we cen set enum JobExecuteTypeInCluster that has two options:
- CONCURRENT - job will be executed parralel on nodes
- NOT_CONCURRENT - job which can run only on one node
information about concurrent jobs is stored in memory
information about not concurrent jobs is stored in database

JIRA:
https://openhubframework.atlassian.net/browse/OHFJIRA-27

pjuza

Quite big implementation :).

I don't have any critical comments, only many smallers. I'm not able to validate the whole model (because it's too big ...) but I believe it's ok ...

pjuza · 2017-03-14T22:02:07Z

core-api/src/main/java/org/openhubframework/openhub/api/common/quartz/ClusterExecuteType.java

+ * @see QuartzSimpleTrigger
+ * @since 2.0
+ */
+public enum ClusterExecuteType {


Wouldn't be better ScheduledJobType?

I dont know. This is not for job, but it is for trigger. And maybe this is not good. I will add this setting into job. And name can be JobExecuteTypeInCluster.

pjuza · 2017-03-14T22:09:23Z

core-api/src/main/java/org/openhubframework/openhub/api/common/quartz/QuartzSimpleTrigger.java

+import org.quartz.SimpleTrigger;
+
+/**
+ * Defined one {@link SimpleTrigger} in {@link QuartzJob}.


Better describe why use this annotation instead of QuartzCronTrigger

pjuza · 2017-03-14T22:15:10Z

...ain/java/org/openhubframework/openhub/api/common/quartz/SimpleTriggerMisfireInstruction.java

+    /**
+     * See documentation in {@link SimpleTrigger#MISFIRE_INSTRUCTION_SMART_POLICY}.
+     *
+     * @see SimpleTrigger


Isn't it needless to add more times "@see SimpleTrigger"?

I will remove it and add "@see SimpleTrigger" on class.

pjuza · 2017-03-14T22:17:06Z

core-spi/src/main/java/org/openhubframework/openhub/spi/quartz/TriggerFactory.java

+/**
+ * Factory for create new {@link Trigger}s instance.
+ * <p>
+ * Every trigger instance running concurrent in all nodes in cluster at the same time


language mistake - Every trigger instance is able to running in ...

Factory for create new => Factory class for creating new ...

pjuza · 2017-03-14T22:21:13Z

core-api/src/main/java/org/openhubframework/openhub/api/common/quartz/QuartzSimpleTrigger.java

+     *
+     * @return execute type in cluster mode
+     */
+    ClusterExecuteType clusterExecuteType();


Why is there not default value? I would expect concurrent ...

I think that default value is not good idea. Set this attribute is important and when he has default value, many user not fill them.

pjuza · 2017-03-14T22:22:48Z

...main/java/org/openhubframework/openhub/core/common/asynch/queue/PartlyFailedMessagesJob.java

+ * Job running concurrent in all nodes in cluster at the same time.
+ * </p>
+ *
+ * @author Petr Juza


I don't know why there are so many @see items in specific implementations, I would remove them.

I will remove it

pjuza · 2017-03-14T22:24:54Z

core-api/src/main/java/org/openhubframework/openhub/api/common/quartz/QuartzJob.java

+     *
+     * @return cron triggers
+     */
+    QuartzCronTrigger[] cronTriggers() default {};


Is it possible to set cron and simple triggers at the same time?
If not then add description to JavaDoc + it's possible to define common parent class/annotation and then have only one trigger parameter?

Yes it is. One job can have one or more QuartzCronTrigger and one or more QuartzSimpleTrigger at the same time.

pjuza · 2017-03-14T22:26:24Z

core/src/main/java/org/openhubframework/openhub/core/common/quartz/OpenHubQuartzJob.java

+
+/**
+ * Defined one quartz {@link Job} with default group name {@value #OPEN_HUB_JOB_GROUP_NAME}.
+ * For base use see in {@link {@link QuartzJob}}.


Remove once @link ...

pjuza · 2017-03-14T22:29:04Z

core-api/src/main/java/org/openhubframework/openhub/api/common/quartz/QuartzJob.java

+     *
+     * @return name
+     */
+    String name() default StringUtils.EMPTY;


I see method in QuartzUtils for generating job's name. Wouldn't be better to define JobNameGenerator interface with default implementation from QuartzUtils?

In QuartzUtils is not method for generate job name, but for generate trigger name. Job name is defined in annotation attribute name. I will create TriggerNameGenerator interface.

pjuza · 2017-03-14T22:36:02Z

core/src/main/java/org/openhubframework/openhub/core/config/QuartzConfig.java

+     */
+    @Bean(name = "quartzProperties")
+    @Profile(Profiles.POSTGRES)
+    public Properties quartzPostgresProperties() {


I would define it in property file because when you add new DB then you should always add new method to the code.

pjuza

I went through all your comments and it's OK for me.

Only one thing - how to solve when new database type will be used? At this moment you should add new code and it's not flexible ...

openhubframework · 2017-03-20T18:06:12Z

You have right, this is not good solution. I will change it and move properties into configuration file application-h2.cfg or application-postgres.cfg.

openhubframework · 2017-03-25T20:50:38Z

It is ready for review.

Changes:

Move quartz settgins from QuartzConfig into application properties
Add name() and group() into QuartzSimpleTrigger and QuartzCronTrigger - now user can direct specify trigger name and trigger group (generator is not necessary)
Edit javadoc (remove @see etc.)
rename ClusterExecuteType into JobExecuteTypeInCluster and add this attribute into QuartzJob (remove from triggers)

coveralls · 2017-03-25T21:08:45Z

Coverage decreased (-6.7%) to 64.474% when pulling 3e2167b on feature/OHFJIRA-27-Add-support-for-clustered-jobs into 0237ab9 on develop.

pjuza

Two common points:

test coverage descreased about 6% - it's too much, please add more unit tests
please add wiki pages with information about using your quartz API. It will be helpful to better understand your code

pjuza · 2017-03-27T22:12:21Z

common/src/main/java/org/openhubframework/openhub/common/Tools.java

+     * @param env                environment
+     * @param propertyNamePrefix prefix for property names
+     * @return {@link Properties} with properties that name start with propertyNamePrefix
+     */


Consider to use Tools.getAllKnownPropertyNames in implementation ...

I will use it. One small disadvantage is, that list will be use twice (first for getting names and second for getting values for names).

pjuza

Once again review ...

pjuza · 2017-03-27T22:17:32Z

core-api/src/main/java/org/openhubframework/openhub/api/common/quartz/QuartzJob.java

+ * {@link QuartzCronTrigger}.
+ * <p>
+ * Annotation can be added on method and this method will be called when trigger will be executed.
+ * </p>


if you have starting and ending element p then it will create two empty lines and you propably want to have one line, don't you?

I dont understand. Where is two empty lines? In HTML or in source code?

When you use ending p and starting p ...

pjuza · 2017-03-27T22:18:46Z

core-api/src/main/java/org/openhubframework/openhub/api/common/quartz/QuartzJob.java

+ * Annotation can be added on method and this method will be called when trigger will be executed.
+ * </p>
+ * <p>
+ * Every job must have at least one {@link QuartzCronTrigger} or {@link QuartzSimpleTrigger}. And one job can have


it's quite complicated sentence :)

pjuza · 2017-03-27T22:22:55Z

...in/java/org/openhubframework/openhub/core/common/quartz/jobfactory/BeanMethodJobFactory.java

+    //-------------------------------------------------- SET / GET -----------------------------------------------------
+
+    @Override
+    public void setApplicationContext(final ApplicationContext applicationContext) {


We have already full AOP support thus you can use @configured ...

I used Autowired for load ApplicationContext property

coveralls · 2017-04-07T19:06:11Z

Coverage decreased (-1.6%) to 55.401% when pulling 3397711 on feature/OHFJIRA-27-Add-support-for-clustered-jobs into e2ddfad on develop.

Issue: OHFJIRA-27

coveralls · 2017-05-03T18:51:56Z

Coverage decreased (-1.6%) to 55.295% when pulling f2e129c on feature/OHFJIRA-27-Add-support-for-clustered-jobs into a31eaee on develop.

hanusto

For this moment these PR meets the requirements.

pjuza requested changes Mar 14, 2017

View reviewed changes

pjuza reviewed Mar 20, 2017

View reviewed changes

openhubframework added this to the v2.0 milestone Mar 20, 2017

openhubframework added the task label Mar 20, 2017

openhubframework force-pushed the feature/OHFJIRA-27-Add-support-for-clustered-jobs branch from 9103f7e to 3e2167b Compare March 25, 2017 20:45

pjuza reviewed Mar 27, 2017

View reviewed changes

openhubframework force-pushed the feature/OHFJIRA-27-Add-support-for-clustered-jobs branch from 9bb7fd1 to 3397711 Compare April 7, 2017 18:52

Add support for clustered jobs (quartz)

f2e129c

Issue: OHFJIRA-27

hanusto force-pushed the feature/OHFJIRA-27-Add-support-for-clustered-jobs branch from 3397711 to f2e129c Compare May 3, 2017 18:37

hanusto self-requested a review May 3, 2017 19:35

hanusto approved these changes May 3, 2017

View reviewed changes

hanusto requested a review from pjuza May 3, 2017 19:39

hanusto merged commit f2ff63b into develop May 3, 2017

hanusto deleted the feature/OHFJIRA-27-Add-support-for-clustered-jobs branch May 3, 2017 19:41

Conversation

openhubframework commented Mar 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pjuza left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pjuza left a comment

Choose a reason for hiding this comment

Uh oh!

openhubframework commented Mar 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openhubframework commented Mar 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Mar 25, 2017

Uh oh!

pjuza left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pjuza left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Apr 7, 2017

Uh oh!

coveralls commented May 3, 2017

Uh oh!

hanusto left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

openhubframework commented Mar 12, 2017 •

edited

Loading

openhubframework commented Mar 20, 2017 •

edited

Loading

openhubframework commented Mar 25, 2017 •

edited

Loading