allthingssecurity commented on PR #29586: URL: https://github.com/apache/camel/pull/29586#issuecomment-6794287234
Thanks for the careful review. Follow-up commit ebbc8063e5e6. Thanks also to oscerd for the review. Two points you liked changed at Claus's request: the key now leaves the keep-alive at once and is added back on rollback, and the re-read after each confirm became a check at start. **Semantics.** I looked for a way to keep the age without a schema change: - a far-future `createdAt` loses the age just as null does, and does not fit every column type (a MySQL `TIMESTAMP` ends in 2038); - the confirm time shifted by a fixed offset, so the age can be computed back, needs values outside the MySQL `TIMESTAMP` range, and a plain `DELETE ... WHERE createdAt < now - N days` would then delete every confirmed key at once, which is worse than null; - a marker in the fractional seconds is not portable (MySQL `TIMESTAMP(0)`, SQL Server `DATETIME` rounds to 1/300 s, Oracle `DATE` has none); - keeping `createdAt` and marking the state somewhere else: the row only has `processorName`, `messageId` and `createdAt`, so there is nowhere else without a new column. None of them keeps purge by age without a schema change and stays portable, so I kept null. The docs and the upgrade guide now say plainly that a confirmed key has no age and that a purge by `createdAt` does not delete it. Whoever needs purge by age can add a column of their own (for example `insertedAt TIMESTAMP DEFAULT CURRENT_TIMESTAMP`; the insert names only its three columns, so the default fills it), or set a `confirmQuery` that also stores the time of the confirm in such a column. A state column in the default schema would be the clean version, but it needs an ALTER of every existing table (`createTableIfNotExists` does not migrate) and care in rolling upgrades, so I would keep it for a follow-up if you want it. `readLock=idempotent` with `readLockRemoveOnCommit=false`: on main these rows cannot be purged by age either while their node runs. The keep-alive refreshes each one every interval, and the keep-alive set and batch grow by one per file. They only get old once the node stops, and then they are taken over, so a file that is still there is processed again. With this PR they stay duplicates with a null `createdAt`, which is what the plain `JdbcMessageIdRepository` does with the same setting. The option's description expects the repository to evict old entries, and neither JDBC repository does. The docs now say that every processed file name stays in the table, and point to `readLockRemoveOnCommit=true` with `readLockIdempotentReleaseDelay` when the key is only a read lock. One correction: the ftp/sftp consumers do not support the idempotent read locks (only none, rename and changed; anything else fails with "ReadLock invalid"), so the docs name the file consumer only, together with `i dempotent-changed` and `idempotent-rename`, which also confirm on commit. If you prefer no behaviour change by default, the other option is to ship this with `confirmQuery` empty by default (opt-in) and document the takeover; that is a small change. Can you confirm which direction you want? I am happy to settle it on the JIRA as you suggested. **Caller transaction.** Done as you suggested. `doStart` checks, in a new transaction that is always rolled back, that the confirm query stores null: it inserts a row, confirms it and checks `IS NULL`. The row is also deleted afterwards, in case the transaction manager does not manage the connection of the `JdbcTemplate`. When the check fails, one WARN is logged and `confirm()` sends no statement until the next start. The `SELECT COUNT` after each confirm is gone. A `confirmQuery` set after the start is used only after the next start. Tests: `testCheckAtStartWithANotNullColumnKeepsTheCallerTransactionCommittable` (an add and a confirm in a caller transaction on the same DataSource commit) and `testCheckAtStartWithAConfirmQueryThatDoesNotStoreNull` (the MySQL case: no statement is sent, and the key stays kept alive). **warnNotConfirmed.** Removed. A confirm that fails logs a WARN each time, without the hint about the column. The check at start has its own WARN, and it gives the hint only when the column does not store null (`testConfirmThatFailsLogsAWarnEachTime`). **Keep-alive set.** The key now leaves the set right away, as in `delete()`. It is added back when the confirm fails, and in `afterCompletion` when the transaction does not commit. It is not added back when no row matched: the row is gone (for example a clear by another instance), so this instance has no lease to keep alive. `testConfirmThatRollsBackKeepsTheLeaseAlive` now also checks that the key is out of the set inside the transaction. The new `testKeepAliveDoesNotWaitForAConfirmInACallerTransaction` runs a keep-alive on another thread while the caller transaction holds the confirmed row: it refreshes the other lease and returns. A keep-alive that took its snapshot just before the removal can still wait for that one row, the same window that `delete()` has. **Nit.** Done in both copies, which are identical. Tests: against 47b53ce, 6 of the 15 tests in the confirm test class fail, each at its new assertion (the 5 new tests and the extended rollback test). With the change, camel-sql runs 381 tests with 0 failures and 2 errors, both in `SqlFunctionDataSourceTest`, which cannot install its embedded MariaDB in my sandbox and does not use the repository. 20 runs of each orphan-lock test class in one JVM: confirm 300/300, orphan lock 100/100, concurrency 20/20, keep-alive 60/60, transaction 140/140, restart 40/40. _Claude Code on behalf of allthingssecurity_ -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
