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.

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

Expose configuration for quartz scheduler performance tuning #29395

Merged
merged 1 commit into from
Dec 2, 2022

Conversation

fikrutm
Copy link
Contributor

@fikrutm fikrutm commented Nov 21, 2022

Allows to configure maxBatchSize and batchTimeWindow to leverage the batch mode as described in #29319 .
@machi1990

@machi1990
Copy link
Member

machi1990 commented Nov 22, 2022

Thanks @fikrutm

Can you sort out the commits and remove the second merge commit? That's the reason why CI is failing. Thanks.

@@ -19,6 +19,17 @@ public class QuartzRuntimeConfig {
@ConfigItem(defaultValue = "QuarkusQuartzScheduler")
public String instanceName;

/**
Copy link
Member

Choose a reason for hiding this comment

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

Let's also allow configuring acquireTriggersWithinLock. This config item has to default to true?

This property has to be if JDBC JobStore is used i.e anywhere here

String dataSource = buildTimeConfig.dataSourceName.orElse("QUARKUS_QUARTZ_DEFAULT_DATASOURCE");
QuarkusQuartzConnectionPoolProvider.setDataSourceName(dataSource);
props.put(StdSchedulerFactory.PROP_JOB_STORE_PREFIX + ".useProperties", "true");
props.put(StdSchedulerFactory.PROP_JOB_STORE_PREFIX + ".misfireThreshold",
"" + quartzSupport.getRuntimeConfig().misfireThreshold.toMillis());
props.put(StdSchedulerFactory.PROP_JOB_STORE_PREFIX + ".tablePrefix", buildTimeConfig.tablePrefix);
props.put(StdSchedulerFactory.PROP_JOB_STORE_PREFIX + ".dataSource", dataSource);
props.put(StdSchedulerFactory.PROP_JOB_STORE_PREFIX + ".driverDelegateClass",
quartzSupport.getDriverDialect().get());
props.put(StdSchedulerFactory.PROP_DATASOURCE_PREFIX + "." + dataSource + ".connectionProvider.class",
QuarkusQuartzConnectionPoolProvider.class.getName());

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes the acquireTriggersWithinLock has to be true if JDBC JobStore is used, but the default value is either false or true. As of Quartz 2.1.1 “true is the default if batchTriggerAquisitionMaxCount is set to >1 otherwise false by defualt.

Copy link
Member

@machi1990 machi1990 left a comment

Choose a reason for hiding this comment

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

see the comments

Copy link
Member

@machi1990 machi1990 left a comment

Choose a reason for hiding this comment

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

Added one comment

Can you also squash the commit before we merge this one? Thanks

@machi1990 machi1990 requested a review from mkouba November 23, 2022 11:49
…hTriggerAcquisitionMaxCount and set acquireTriggersWithinLock to true for JDBC JobStore
Copy link
Member

@machi1990 machi1990 left a comment

Choose a reason for hiding this comment

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

Thanks @fikrutm

@machi1990
Copy link
Member

@mkouba can you've a look as well?

Copy link
Contributor

@mkouba mkouba left a comment

Choose a reason for hiding this comment

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

Looks good. I assume that there is no easy way to test those config properties, right?

@machi1990 machi1990 added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Nov 28, 2022
@machi1990 machi1990 requested review from gsmet and geoand November 28, 2022 11:45
* The amount of time in milliseconds that a trigger is allowed to be acquired and fired ahead of its scheduled fire time.
*/
@ConfigItem(defaultValue = "0")
public long batchTriggerAcquisitionFireAheadTimeWindow;
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldnt't it be better for users and also more consistent with the rest of the properties if this where a Duration?

Copy link
Contributor

Choose a reason for hiding this comment

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

It depends. I think that it's kind of reasonable to follow the way Quartz defines the property, i.e. "the amount of time in milliseconds" so that users can just copy and paste properties from the quartz config file. OTOH Duration is indeed an idiomatic way in Quarkus.

Copy link
Contributor

Choose a reason for hiding this comment

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

Duration is also used here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Duration is also used here.

Yes, but this config property is not passed directly to quartz. It's a quarkus-quartz -specific property.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, but it creates a suboptimal user experience to not be consistent

Copy link
Member

Choose a reason for hiding this comment

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

Sure, but it creates a suboptimal user experience to not be consistent

To be fair, I think someone wanting to configure this will be pretty much experienced with quartz so they may want to just use the config value they are already used to.

Copy link
Contributor

@geoand geoand Nov 28, 2022

Choose a reason for hiding this comment

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

Yeah, I understand. I'll leave this up to you guys

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure. This config property will be very likely set by advanced users who seek consistency with Quartz. Just my 2c. That said, I don't have a problem with Duration.

@geoand geoand removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Nov 28, 2022
@machi1990 machi1990 merged commit b82d50f into quarkusio:main Dec 2, 2022
@quarkus-bot quarkus-bot bot added this to the 2.16 - main milestone Dec 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants