Conversation
|
@codex please review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6aff97d2dd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| fun evolve(ex: Exception?): Evolution = | ||
| copy( | ||
| evolutions = evolutions + 1, | ||
| retriesRemaining = retriesRemaining?.let { maxOf(it - 1, 0) }, | ||
| delay = min(delay.multipliedBy(config.backoffFactor.toLong()), config.maxDelay), | ||
| thrownExceptions = ex?.let { listOf(it) + thrownExceptions } ?: thrownExceptions, |
There was a problem hiding this comment.
The retry config explicitly allows non-integer backoffFactor values (e.g., 1.5), but the delay evolution truncates it via toLong(). That makes any factor between 1.0 and 1.999 behave as 1, so delays never grow and retries can become much more aggressive than configured (and never reach maxDelay as expected). This affects any caller using fractional backoff values and can increase load or contention during transient failures. Consider multiplying by the double factor instead of truncating it.
Useful? React with 👍 / 👎.
delayedqueue-jvm/src/main/kotlin/org/funfix/delayedqueue/jvm/DelayedQueueJDBC.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
This PR introduces JDBC-based DelayedQueue support with HSQLDB as the first database backend. The implementation includes comprehensive retry logic, exception handling, database migrations, and vendor-specific SQL adapters. The PR also transitions from SQLException to ResourceUnavailableException in public APIs for better abstraction.
Changes:
- Adds
DelayedQueueJDBCwith HSQLDB support, including SQL vendor adapters, exception filters, and retry mechanisms - Introduces
ResourceUnavailableExceptionas a checked exception replacingSQLExceptionin public APIs - Adds
RetryConfigfor configuring exponential backoff retry logic - Updates all tests from SQLite references to HSQLDB (though SQLite driver still exists in the enum for future use)
Reviewed changes
Copilot reviewed 41 out of 41 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
gradle/libs.versions.toml |
Adds HSQLDB JDBC driver dependency |
delayedqueue-jvm/src/main/kotlin/org/funfix/delayedqueue/jvm/DelayedQueueJDBC.kt |
Main JDBC implementation with retry logic and optimistic locking |
delayedqueue-jvm/src/main/kotlin/org/funfix/delayedqueue/jvm/internals/jdbc/SQLVendorAdapter.kt |
Database-specific SQL query generation with parameterized statements |
delayedqueue-jvm/src/main/kotlin/org/funfix/delayedqueue/jvm/internals/jdbc/HSQLDBMigrations.kt |
HSQLDB schema migrations with indexes |
delayedqueue-jvm/src/main/kotlin/org/funfix/delayedqueue/jvm/internals/jdbc/SqlExceptionFilters.kt |
Exception classification for retry logic |
delayedqueue-jvm/src/main/kotlin/org/funfix/delayedqueue/jvm/RetryConfig.kt |
Retry configuration with exponential backoff |
delayedqueue-jvm/src/main/kotlin/org/funfix/delayedqueue/jvm/JdbcDriver.kt |
Adds HSQLDB to driver enum |
delayedqueue-jvm/src/main/kotlin/org/funfix/delayedqueue/jvm/DelayedQueue.kt |
Updates exception declarations from SQLException to ResourceUnavailableException |
delayedqueue-jvm/src/main/kotlin/org/funfix/delayedqueue/jvm/CronConfigHash.kt |
Fixes hash generation to match Scala format with leading newlines |
delayedqueue-jvm/src/main/kotlin/org/funfix/delayedqueue/jvm/CronDailySchedule.kt |
Ensures getNextTimes always returns at least one time |
delayedqueue-jvm/src/main/kotlin/org/funfix/delayedqueue/jvm/internals/utils/raise.kt |
Renames sneakyRaises to unsafeSneakyRaises with extensive documentation |
| Test files (Java) | Comprehensive API contract tests using HSQLDB |
| Test files (Kotlin) | Internal implementation tests for retry logic, exception filters |
AGENTS.md |
Adds guidelines about following original implementations and testing conventions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
delayedqueue-jvm/src/main/kotlin/org/funfix/delayedqueue/jvm/RetryConfig.kt
Outdated
Show resolved
Hide resolved
|
@copilot review this PR from the perspective of exporting a stable API targeting Java projects and Java developers. Are there code-related problems? Are any best practices violated (as outlined in AGENTS.md or in the installed Have a look at |
|
@alexandru I've opened a new pull request, #3, to work on those changes. Once the pull request is ready, I'll request review from you. |
Add 'id' column to KindPlusScheduledAtIndex in all database migrations to ensure ORDER BY (scheduledAt, id) queries are fully index-covered. Also add migration #2 to update existing databases by dropping and recreating the index with the additional column. This ensures both deterministic ordering and optimal query performance without requiring additional sort operations. Co-authored-by: alexandru <11753+alexandru@users.noreply.github.com>
Introduces
DelayedQueueJDBCbased on HSQLDB.In a future PR, MS-SQL support is planned.