Scheduler-expression compatible `Duration` converter

[knutwannheden]

Description

The scheduler's @Scheduled annotation allows config expressions in its every and cron attributes. For both of the values off and disabled are also allowed.

For Quarkus-extensions which include @Scheduled annotated classes and document their schedules using an @ConfigItem I think it would make sense for Quarkus to include a Converter<Duration> implementation, which doesn't choke on off and disabled. Otherwise the whole mechanism doesn't work, since SmallRye Config will fail to parse the config at startup.

Should possibly even the already provided standard DurationConverter support off and disabled? If yes, I suppose it would have to return null in those cases.

Implementation ideas

No response

[quarkus-bot[bot]]

/cc @mkouba

[knutwannheden]

/cc @radcortez

[mkouba]

For Quarkus-extensions which include @Scheduled annotated classes and document their schedules using an @ConfigItem...

Hm, I'm not sure I fully understand. I mean the @Scheduled annotation is primarily intented for application developers and not for extensions authors. So in your particular case you have a scheduled method (e.g. @Scheduled(every = "${myConfig.foo}")) in an extension class that is added as an additional bean archive during the discovery and then you have MyConfig with @ConfigItem foo to generate the docs automatically?

[knutwannheden]

Hm, I'm not sure I fully understand. I mean the @Scheduled annotation is primarily intented for application developers and not for extensions authors. So in your particular case you have a scheduled method (e.g. @Scheduled(every = "${myConfig.foo}")) in an extension class that is added as an additional bean archive during the discovery and then you have MyConfig with @ConfigItem foo to generate the docs automatically?

Yes, I suppose this is a bit unusual. We indeed have a Quarkus extension which includes a scheduled method. The extension documents its config properties using @ConfigItem. I was hoping that downstream applications would be able to set the property used by the every-expression to off. However, the DurationConverter cannot handle this.

If you consider this case to be too unusual, I can close this issue and implement my own converter.

[mkouba]

If you consider this case to be too unusual, I can close this issue and implement my own converter.

Yes, I think that it's a bit unusual.

Also org.eclipse.microprofile.config.spi.Converter.convert(String) must not return null so I wonder how would you represent the off/disabled duration value?

You can try to make your config property a String and then parse it manually, i.e.:

OptionalLong duration = SchedulerUtils.parseEveryAsMillis(new Scheduled() {
// implement all methods
});

[knutwannheden]

Also org.eclipse.microprofile.config.spi.Converter.convert(String) must not return null so I wonder how would you represent the off/disabled duration value?

From the Javadoc:

@return the converted value, or {@code null} if the value is empty

so it seems like returning null should be allowed.

[mkouba]

Also org.eclipse.microprofile.config.spi.Converter.convert(String) must not return null so I wonder how would you represent the off/disabled duration value?

From the Javadoc:

@return the converted value, or {@code null} if the value is empty

so it seems like returning null should be allowed.

Ah, sorry it's the argument that must not be null... OTOH off does not mean that the value is "empty", e.g. if you have @Scheduled(every = "${my.every.property:10s}") there's a difference between my.every.property=off (scheduled method is not executed at all) and my.every.property not set at all - "empty" (executed every 10s).

[mkouba]

Should possibly even the already provided standard DurationConverter support off and disabled? If yes, I suppose it would have to return null in those cases.

I was thinking about it and I don't think that we should support off and disabled in the io.quarkus.runtime.configuration.DurationConverter because it's a scheduler-specific trait. I.e. a regular duration cannot be off/disabled. It's the scheduled method that could be off/disabled.

The question is whether and how to support the use case described in this issue :shrug:.

If you consider this case to be too unusual, I can close this issue and implement my own converter.

Yes, a custom Converter with a priority higher than io.quarkus.runtime.configuration.DurationConverter seems like an acceptable solution.

WDYT? @knutwannheden @radcortez

[knutwannheden]

A custom converter is the solution I implemented. The only improvement over that I can think of is if Quarkus provides a reusable converter.

[radcortez]

Should this case use a Duration even? Probably, it requires a new type that adjusts to a schedule configuration and its own converter.

[mkouba]

Should this case use a Duration even? Probably, it requires a new type that adjusts to a schedule configuration and its own converter.

That is a good point.

We use the same logic for both Scheduled#every() and Scheduled#cron(). So it should be probably two types and two converters. Maybe ScheduledDuration and ScheduledCron?

[radcortez]

Sure, but don't ask me for names. I'm terrible at naming :)

[mkouba]

Actually, we can't provide a simple converter for cron expressions because Converter is a service provider (i.e. non-contextual object) but in quarkus-scheduler the quarkus.scheduler.cron-type config property defines the syntax used in expressions. I mean, it's probably doable but not worth the effort.

[radcortez]

You could use a config customizer to register a converter programatically (with a delegate): https://smallrye.io/smallrye-config/3.9.1/config/customizer/

And a factory to read the config: https://smallrye.io/smallrye-config/3.9.1/config-sources/factories/

Then, statically assign or modify the delegate to provide the converter configuration.

Source factories run before mappings / roots are populated, so when the converter is required, it should be ready to be used.

[mkouba]

@knutwannheden Just to clarify - do you really need to use Duration as the type for the config item? I mean, if you use String then it should not fail and you can document the config item properly. The only difference is that the default hint for Duration would not be displayed. Also I don't think that anything special is displayed in the docs for a custom type. So even if we provide a special type/converter it won't help much IMO...

[knutwannheden]

I understand. We do in fact use the generated documentation, but living without that duration hint is fine.

[mkouba]

I understand. We do in fact use the generated documentation, but living without that duration hint is fine.

Ok, I will close this issue as "not planned" because I don't believe it's worth the effort.

However, thanks to this issue I've identified a bug in cron syntax conversion; should be fixed in https://github.com/quarkusio/quarkus/pull/43487. So thanks for reporting @knutwannheden!