From ea14d10c56494c1dbb080c4e3aace002940dd8d0 Mon Sep 17 00:00:00 2001 From: lcabancla Date: Tue, 12 Nov 2024 12:48:03 -0500 Subject: [PATCH] ## Description This PR introduces a cronPattern parameter to CronRunnableEntry and CronEntryModule, enabling clients to specify a custom cron pattern directly when registering a runnable, rather than relying solely on annotations. This change maintains backward compatibility by setting cronPattern to null by default, preserving existing behavior. ### Key Changes - CronRunnableEntry Update: Added an optional `cronPattern` property with a default value of `null`. This allows clients to specify a cron pattern directly when creating the entry. - CronEntryModule Update: Modified `CronEntryModule` to accept an optional `cronPattern` parameter in its factory methods, enabling users to provide a cron pattern when registering a runnable. - `CronService::startUp` Logic: Enhanced startUp to prioritize the cronPattern from `CronRunnableEntry`. If no pattern is specified, the service falls back to the `@CronPattern` annotation on the runnable class. This update improves flexibility, allowing clients to override the annotation pattern with a custom pattern if needed. ### Compatibility - Backward Compatibility: Existing clients that rely on annotations for cron scheduling will continue to work without modification, as cronPattern defaults to null. - Enhanced Flexibility: Clients now have the option to specify a custom cron pattern directly, without changing the underlying annotations. ### Use Cases This enhancement is particularly valuable for cron jobs that need flexible scheduling across different contexts. For instance, a database cleanup job may require frequent execution for services with high insertion rates, ensuring timely data management and optimal performance. Conversely, for services with low insertion rates, the job can be scheduled less frequently, conserving resources while still meeting maintenance needs. ### Additional Notes - Updated comments in `CronService::startUp` to clarify the precedence between cronPattern in `CronRunnableEntry` and the @CronPattern annotation. - This change is additive and introduces no breaking changes for existing clients. GitOrigin-RevId: 838c362bd0c589486d3dee12dd24455c40d0940f --- misk-cron/api/misk-cron.api | 5 +++-- .../main/kotlin/misk/cron/CronEntryModule.kt | 13 +++++++------ .../src/main/kotlin/misk/cron/CronService.kt | 8 +++++++- .../src/test/kotlin/misk/cron/CronTest.kt | 18 ++++++++++++++++++ 4 files changed, 35 insertions(+), 9 deletions(-) diff --git a/misk-cron/api/misk-cron.api b/misk-cron/api/misk-cron.api index 9f031571448..0001ff47f7f 100644 --- a/misk-cron/api/misk-cron.api +++ b/misk-cron/api/misk-cron.api @@ -1,10 +1,11 @@ public final class misk/cron/CronEntryModule : misk/inject/KAbstractModule { public static final field Companion Lmisk/cron/CronEntryModule$Companion; - public synthetic fun (Lkotlin/reflect/KClass;Lkotlin/jvm/internal/DefaultConstructorMarker;)V + public synthetic fun (Lkotlin/reflect/KClass;Lmisk/cron/CronPattern;Lkotlin/jvm/internal/DefaultConstructorMarker;)V } public final class misk/cron/CronEntryModule$Companion { - public final fun create (Lkotlin/reflect/KClass;)Lmisk/cron/CronEntryModule; + public final fun create (Lkotlin/reflect/KClass;Lmisk/cron/CronPattern;)Lmisk/cron/CronEntryModule; + public static synthetic fun create$default (Lmisk/cron/CronEntryModule$Companion;Lkotlin/reflect/KClass;Lmisk/cron/CronPattern;ILjava/lang/Object;)Lmisk/cron/CronEntryModule; } public final class misk/cron/CronManager { diff --git a/misk-cron/src/main/kotlin/misk/cron/CronEntryModule.kt b/misk-cron/src/main/kotlin/misk/cron/CronEntryModule.kt index 952efecc689..d980d5cc32f 100644 --- a/misk-cron/src/main/kotlin/misk/cron/CronEntryModule.kt +++ b/misk-cron/src/main/kotlin/misk/cron/CronEntryModule.kt @@ -3,23 +3,24 @@ package misk.cron import misk.inject.KAbstractModule import kotlin.reflect.KClass -internal class CronRunnableEntry(val runnableClass: KClass) +internal class CronRunnableEntry @JvmOverloads constructor(val runnableClass: KClass, val cronPattern: CronPattern? = null) class CronEntryModule private constructor( - private val runnableClass: KClass + private val runnableClass: KClass, + private val cronPattern: CronPattern? = null ) : KAbstractModule() { override fun configure() { - multibind().toInstance(CronRunnableEntry(runnableClass)) + multibind().toInstance(CronRunnableEntry(runnableClass, cronPattern)) } companion object { - inline fun create(): CronEntryModule = create(A::class) + inline fun create(cronPattern: CronPattern? = null): CronEntryModule = create(A::class, cronPattern) /** * Registers a cron runnable. * @param cronRunnableClass: The cron runnable to register. */ - fun create(cronRunnableClass: KClass): CronEntryModule { - return CronEntryModule(cronRunnableClass) + fun create(cronRunnableClass: KClass, cronPattern: CronPattern? = null): CronEntryModule { + return CronEntryModule(cronRunnableClass, cronPattern) } } } diff --git a/misk-cron/src/main/kotlin/misk/cron/CronService.kt b/misk-cron/src/main/kotlin/misk/cron/CronService.kt index bc9bf64231e..8b9644a00cc 100644 --- a/misk-cron/src/main/kotlin/misk/cron/CronService.kt +++ b/misk-cron/src/main/kotlin/misk/cron/CronService.kt @@ -19,7 +19,13 @@ internal class CronService @Inject constructor( cronRunnableEntries.forEach { cronRunnable -> val name = cronRunnable.runnableClass.qualifiedName!! val annotations = cronRunnable.runnableClass.annotations - val cronPattern = annotations.find { it is CronPattern } as? CronPattern + val annotationPattern = annotations.find { it is CronPattern } as? CronPattern + // Use the cron pattern specified in CronRunnableEntry if available. + // If it's null, fall back to the pattern from the @CronPattern annotation on the class. + // This allows the annotation pattern to serve as a default, while specifying + // a cron pattern in CronRunnableEntry will override it if provided. + val cronPattern = cronRunnable.cronPattern + ?: annotationPattern ?: throw IllegalArgumentException("Expected $name to have @CronPattern specified") val runnable = injector.getProvider(cronRunnable.runnableClass.java).get() diff --git a/misk-cron/src/test/kotlin/misk/cron/CronTest.kt b/misk-cron/src/test/kotlin/misk/cron/CronTest.kt index 52d273ca2d2..dd94820615e 100644 --- a/misk-cron/src/test/kotlin/misk/cron/CronTest.kt +++ b/misk-cron/src/test/kotlin/misk/cron/CronTest.kt @@ -30,6 +30,8 @@ class CronTest { install(CronEntryModule.create()) install(CronEntryModule.create()) install(CronEntryModule.create()) + // Override ConfigurableHourCron's schedule to every minute. + install(CronEntryModule.create(cronPattern = CronPattern("* * * * *"))) } } @@ -41,6 +43,7 @@ class CronTest { @Inject private lateinit var minuteCron: MinuteCron @Inject private lateinit var hourCron: HourCron @Inject private lateinit var throwsExceptionCron: ThrowsExceptionCron + @Inject private lateinit var configurableHourCron: ConfigurableHourCron private lateinit var lastRun: Instant @@ -59,15 +62,20 @@ class CronTest { assertThat(minuteCron.counter).isEqualTo(0) assertThat(hourCron.counter).isEqualTo(0) assertThat(throwsExceptionCron.counter).isEqualTo(0) + assertThat(configurableHourCron.counter).isEqualTo(0) // Advance one hour in one minute intervals. repeat(60) { clock.add(Duration.ofMinutes(1)) runCrons() } + assertThat(minuteCron.counter).isEqualTo(60) assertThat(throwsExceptionCron.counter).isEqualTo(4) assertThat(hourCron.counter).isEqualTo(1) + // configurableHourCron has @CronPattern configured to hourly, but is overridden + // to every minute by CronEntryModule::create(). + assertThat(configurableHourCron.counter).isEqualTo(60) } @Test @@ -114,6 +122,16 @@ class HourCron @Inject constructor() : Runnable { } } +@Singleton +@CronPattern("0 * * * *") // Default cron schedule of hourly. +class ConfigurableHourCron @Inject constructor() : Runnable { + var counter = 0 + + override fun run() { + counter++ + } +} + @Singleton @CronPattern("1/15 * * * *") class ThrowsExceptionCron @Inject constructor() : Runnable {