Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

add scheduler configuration and add support for clustered jobs #3783

Closed
wants to merge 1 commit into from
Closed

add scheduler configuration and add support for clustered jobs #3783

wants to merge 1 commit into from

Conversation

machi1990
Copy link
Member

Fixes #3520

/cc @masini this could interest you :-)

@machi1990 machi1990 force-pushed the 3520 branch 4 times, most recently from bc49474 to 11407ee Compare August 30, 2019 20:26
@geoand
Copy link
Contributor

geoand commented Aug 30, 2019

Some tests seem to be failing. Perhaps you would like to take a look @machi1990 ?

@geoand
Copy link
Contributor

geoand commented Aug 30, 2019

I know @mkouba should have things to say about this :)

@masini
Copy link
Contributor

masini commented Aug 30, 2019

Thank you for your help !

Hope to see this released soon.

@machi1990
Copy link
Member Author

I have a native test failure:

com.oracle.svm.core.jdk.UnsupportedFeatureError: ObjectOutputStream.writeObject()
	at com.oracle.svm.core.util.VMError.unsupportedFeature(VMError.java:102)
	at java.io.ObjectOutputStream.writeObject(ObjectOutputStream.java:69)
	at org.quartz.impl.jdbcjobstore.StdJDBCDelegate.serializeObject(StdJDBCDelegate.java:3056)
	at org.quartz.impl.jdbcjobstore.StdJDBCDelegate.serializeJobData(StdJDBCDelegate.java:3081)
	at org.quartz.impl.jdbcjobstore.StdJDBCDelegate.insertJobDetail(StdJDBCDelegate.java:606)
	at org.quartz.impl.jdbcjobstore.JobStoreSupport.storeJob(JobStoreSupport.java:1117)
	at org.quartz.impl.jdbcjobstore.JobStoreSupport$2.executeVoid(JobStoreSupport.java:1067)
	at org.quartz.impl.jdbcjobstore.JobStoreSupport$VoidTransactionCallback.execute(JobStoreSupport.java:3780)
	at org.quartz.impl.jdbcjobstore.JobStoreSupport$VoidTransactionCallback.execute(JobStoreSupport.java:3778)
	at org.quartz.impl.jdbcjobstore.JobStoreSupport.executeInNonManagedTXLock(JobStoreSupport.java:3864)
	at org.quartz.impl.jdbcjobstore.JobStoreTX.executeInLock(JobStoreTX.java:93)
	at org.quartz.impl.jdbcjobstore.JobStoreSupport.storeJobAndTrigger(JobStoreSupport.java:1063)
	at org.quartz.core.QuartzScheduler.scheduleJob(QuartzScheduler.java:855)
	at org.quartz.impl.StdScheduler.scheduleJob(StdScheduler.java:249)

@dmlloyd is this related to #2656?

I am not sure what's the best way to workaround that since GraalVM does not support Serialization yet? see oracle/graal#460

@bobmcwhirter @dmlloyd @cstancu Any ideas of a workaround you might have will be appreciated.

@machi1990
Copy link
Member Author

I have a native test failure:

com.oracle.svm.core.jdk.UnsupportedFeatureError: ObjectOutputStream.writeObject()
	at com.oracle.svm.core.util.VMError.unsupportedFeature(VMError.java:102)
	at java.io.ObjectOutputStream.writeObject(ObjectOutputStream.java:69)
	at org.quartz.impl.jdbcjobstore.StdJDBCDelegate.serializeObject(StdJDBCDelegate.java:3056)
	at org.quartz.impl.jdbcjobstore.StdJDBCDelegate.serializeJobData(StdJDBCDelegate.java:3081)
	at org.quartz.impl.jdbcjobstore.StdJDBCDelegate.insertJobDetail(StdJDBCDelegate.java:606)
	at org.quartz.impl.jdbcjobstore.JobStoreSupport.storeJob(JobStoreSupport.java:1117)
	at org.quartz.impl.jdbcjobstore.JobStoreSupport$2.executeVoid(JobStoreSupport.java:1067)
	at org.quartz.impl.jdbcjobstore.JobStoreSupport$VoidTransactionCallback.execute(JobStoreSupport.java:3780)
	at org.quartz.impl.jdbcjobstore.JobStoreSupport$VoidTransactionCallback.execute(JobStoreSupport.java:3778)
	at org.quartz.impl.jdbcjobstore.JobStoreSupport.executeInNonManagedTXLock(JobStoreSupport.java:3864)
	at org.quartz.impl.jdbcjobstore.JobStoreTX.executeInLock(JobStoreTX.java:93)
	at org.quartz.impl.jdbcjobstore.JobStoreSupport.storeJobAndTrigger(JobStoreSupport.java:1063)
	at org.quartz.core.QuartzScheduler.scheduleJob(QuartzScheduler.java:855)
	at org.quartz.impl.StdScheduler.scheduleJob(StdScheduler.java:249)

@dmlloyd is this related to #2656?

I am not sure what's the best way to workaround that since GraalVM does not support Serialization yet? see oracle/graal#460

@bobmcwhirter @dmlloyd @cstancu Any ideas of a workaround you might have will be appreciated.

I have pushed new changes that de-activates database job stores in Native Image thus defaulting to ram-store.

@mkouba
Copy link
Contributor

mkouba commented Sep 2, 2019

Hey @machi1990, this looks like a great contribution. However, I think that we'll need to create a separate Quartz extension because the Scheduler extension API/config should not be bound to a Quartz-specific config/API. In fact, we should probably define an SPI in the scheduler extension and implement this SPI in the Quartz extension. One problem with this approach could be that a user would have to declare two dependencies instead of one (quarkus-scheduler + quarkus-quartz VS quarkus-scheduler).

@emmanuelbernard @dmlloyd WDYT?

@emmanuelbernard
Copy link
Member

I think the core question we should open up in quarkus-dev is do we want to keep a simple scheduler API which would not use Quarz (or abstracted of at least) to keep things simple, offer build time improvements possibilities etc
Or do we see Quarkz as the leader and just go with it.

@geoand
Copy link
Contributor

geoand commented Sep 2, 2019

Also, if Quartz's database capabilities can't work (yet) on native, does it make sense to have it exposed?

@machi1990 machi1990 added the triage/on-ice Frozen until external concerns are resolved label Sep 2, 2019
emmanuelbernard
emmanuelbernard requested changes Oct 3, 2019
View reviewed changes
Copy link
Member

@emmanuelbernard emmanuelbernard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ahve strong reservations for two main reasons:

  • no support for native image
  • configuration do leak the fact that it's a blocking implementation and specific to Quartz

An alternative if people really need it and are ok with JVM only would be to fork that extension and host it in a different repo. We wouldn't having in the Quarkus platform but you could reference it in your pom like this one and use it.

docs/src/main/asciidoc/scheduled-guide.adoc
Defaults to: `DefaultQuartzScheduler` +


`quarkus.scheduler.state-store.misfire-threshold`:: The number of duration by which a trigger must have missed its next-fire-time, in order for it to be considered "misfired" and thus have its misfire instruction applied.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could see that one being a functional setting. Is that very useful in practice ? Do we need to expose it ?

docs/src/main/asciidoc/scheduled-guide.adoc
Defaults to: `1m` +


`quarkus.scheduler.state-store.cluster-checking-interval`:: The frequency at which this instance "checks-in" with the other instances of the cluster .This affects the rate of detecting failed instances.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That one goes a bit into the weeds of implementation details. Do we have to have that option expose?

docs/src/main/asciidoc/scheduled-guide.adoc
Type: `java.lang.Boolean` +


`quarkus.scheduler.state-store.driver-delegate-class`:: The JDBC driver delegate class. This is not required if the <<state-store-type, state store>> is `in-memory`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not fan of that, We must pass the quarkus.database name we want to use, not define a JDBC driver and all. That would be the quarkus unified way.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I take that back, what is that delegate class needed? What would people put as value in practice?

docs/src/main/asciidoc/scheduled-guide.adoc
Possible values can be obtained from https://github.com/quartz-scheduler/quartz/tree/master/quartz-core/src/main/java/org/quartz/impl/jdbcjobstore[quartz-scheduler JDBC stores delegate]
+
Type: `java.lang.String` +
Defaults to: `org.quartz.impl.jdbcjobstore.StdJDBCDelegate` +
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

defaulting to Quartz means you say your engine is Quarkz

docs/src/main/asciidoc/scheduled-guide.adoc
- If set to `in-memory`, the scheduler will use the `org.quartz.simpl.RAMJobStore` job store class
- If set to `jdbc`, the scheduler will use the `org.quartz.impl.jdbcjobstore.JobStoreTX` job store class.
When using this option make sure that you have the link:datasource-guide.html[agroal datasource configured] and that <<creating-scheduling-job, scheduling tables>> exists.
This store type is not supported in Native Image because of https://github.com/oracle/graal/issues/460[unsupported Object serialization].
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hum, now that's a line we never crossed. We have our features supported in native, all of them. To me that's a blocker. If people really need this feature we could host a fork of that extension that only work with JVM and one can reference it.

docs/src/main/asciidoc/scheduled-guide.adoc
Defaults to: `in-memory` +


`quarkus.scheduler.instance-id`:: The instance id of the scheduler. This is highly required when running clustered schedulers as each node in the cluster MUST have a unique instanceId.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't auto good enough?

docs/src/main/asciidoc/scheduled-guide.adoc
Defaults to: `AUTO` +


`quarkus.scheduler.thread-count`:: The size of scheduler thread pool. This will initialise the number of worker threads in the pool
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we really need a thread pool exposed. Again back to the implementation abstraction, we could ahve a non blocking one demanding no thread.

docs/src/main/asciidoc/scheduled-guide.adoc
[[creating-scheduling-job]]
.About creating database tables when using `jdbc` state store
=====
The Quarkus scheduler does not create the necessary scheduling tables in database automatically. If these tables are missing, the schuduler will throw an excpetion during application startup.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/schuduler/scheduler/

@emmanuelbernard
Copy link
Member

I only reviewed the doc. Not the rest.

@geoand
Copy link
Contributor

geoand commented Oct 3, 2019

I ahve strong reservations for two main reasons:

  • no support for native image
  • configuration do leak the fact that it's a blocking implementation and specific to Quartz

An alternative if people really need it and are ok with JVM only would be to fork that extension and host it in a different repo. We wouldn't having in the Quarkus platform but you could reference it in your pom like this one and use it.

+1 on this idea from me

@machi1990
Copy link
Member Author

Let me close this since it lacks native support. @emmanuelbernard @geoand

An alternative if people really need it and are ok with JVM only would be to fork that extension and host it in a different repo. We wouldn't having in the Quarkus platform but you could reference it in your pom like this one and use it.

@masini thanks for your interest in having this feature, if you okay with having JVM only, could you please try Emmanuel suggestion above?.

@machi1990 machi1990 closed this Oct 7, 2019
@gsmet gsmet added the triage/invalid This doesn't seem right label Oct 8, 2019
@masini
Copy link
Contributor

masini commented Oct 16, 2019 via email

@machi1990
Copy link
Member Author

machi1990 commented Nov 15, 2019 •
edited
Loading

I'll revive this PR with #5481.

I have opened up #5529

As for the native part, I have been to workaround this by using Properties instead (in jVM and native mode) - https://github.com/quarkusio/quarkus/pull/5529/files#diff-f3233ae68279acfe2869eaffaeb10682R248 and https://github.com/quarkusio/quarkus/pull/5529/files#diff-4cf9981ea9b1aafb626d63db64005fc8R54.

@emmanuelbernard @mkouba @geoand @masini

@machi1990 machi1990 mentioned this pull request Nov 16, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kenfinnigan kenfinnigan kenfinnigan left review comments

@emmanuelbernard emmanuelbernard emmanuelbernard requested changes

Assignees
No one assigned
Labels
area/scheduler triage/invalid This doesn't seem right triage/on-ice Frozen until external concerns are resolved
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make Quartz Extension configurable for clustered scheduler engine
7 participants
@machi1990 @geoand @masini @mkouba @emmanuelbernard @kenfinnigan @gsmet