Repository navigation
Analyzer retry: Persist per-table retry timing via available_at and attempts_since_success columns - #874
Analyzer retry: Persist per-table retry timing via available_at and attempts_since_success columns#874abhisheknath2011 wants to merge 4 commits into
Conversation
…ttempts_since_success columns
| return false; | ||
| } | ||
| return createPending(analyzer, table); | ||
| return createPending(analyzer, table, nextAttemptsSinceSuccess(currentOp, latestHistory)); |
There was a problem hiding this comment.
Can we retry by updating the existing queue row, as discussed in #812, instead of inserting another row and copying counters? Claiming should increment the attempt number and set when ownership expires in one database update. Completion or expiry should update that row only if the same attempt still owns it, then set when it can run again.
Please include fencing to reject callbacks from older attempts, and save each attempt’s result separately. These changes need to land together. We can then remove the code that reconstructs retry counts from history and inserts replacement rows. Heartbeats and preventing duplicate work can follow.
There was a problem hiding this comment.
The current model insert multiple rows (table_UUID, operation type) in the table_operations table as opposed to having single row with PK. I have shared a doc offline to compare multiple rows vs single row with PK. This current model already satisfies the use cases mentioned in the PR #812 and more flexible for changes in future. So the required fixes in the current model and the remaining fixes are associated to changing the design to move to PK (table_UUID, operation type) based model. So we don't need to change the design to move to single row PK .
| -- and reset on success. Lets the retry backoff count consecutive non-successes directly on the row, | ||
| -- independent of the history windows (which can misalign and miscount a recorded success as a | ||
| -- timeout). NOT NULL with a default so existing rows start a fresh streak. | ||
| ALTER TABLE table_operations ADD COLUMN attempts_since_success INT NOT NULL DEFAULT 0; |
There was a problem hiding this comment.
do we need the alter table or can we jsut drop and re-create?
There was a problem hiding this comment.
It should be alter table if new columns are added as per flyway doc and repo flyway doc.
| private String jobId; | ||
|
|
||
| /** Attempt number since the last success; 1 for the first attempt after a success. */ | ||
| private int attemptsSinceSuccess; |
There was a problem hiding this comment.
This should just be attempts. If its successful it stops incrementing.
There was a problem hiding this comment.
Renamed to attempts in all the relevant places.
| /** Job ID returned by the Jobs Service after the scheduler submitted; null until SCHEDULED. */ | ||
| private String jobId; | ||
|
|
||
| /** Attempt number since the last success; 1 for the first attempt after a success. */ |
There was a problem hiding this comment.
It just starts at 1. ITs possible the previous job failed, so this can be collapsed to attempts.
There was a problem hiding this comment.
Yes updated to attempts.
| * database must end up with the optimizer tables that Flyway creates in an empty one, without changing | ||
| * any HouseTables table definition or row. | ||
| * database must end up with the optimizer tables that Flyway creates in an empty one, without | ||
| * changing any HouseTables table definition or row. |
There was a problem hiding this comment.
nit. file changed and only changed whitespace
There was a problem hiding this comment.
Dropped from the PR diff via the main merge — OptimizerSchemaMigrationsTest now matches main
| TableStatsRow statsEntity = | ||
| TableStatsRow.builder().tableUuid("uuid-1").databaseName(DB).tableName("tbl1").build(); | ||
| TableDto expectedTable = TableDto.fromRow(statsEntity); | ||
| // Same dead job, but available_at is already set (stamped on an earlier pass). The gate — not a |
There was a problem hiding this comment.
I don't grok the phrase "stamped" /understand what its saying. The verbage is leaking into the variable names. Can we clarify?
There was a problem hiding this comment.
stampExpiryBackoff → recordExpiryBackoff; "stamp" removed from comments, test names, and variables
…mp" wording - Rename attempts_since_success -> attempts everywhere (column, entity, DTO, analyzer, service, tests). Behavior is unchanged (reset to 1 on success, incremented per retry); the name is simplified as requested. - Fold the attempts and available_at columns into the base create migration (V20261008021832) and delete the two ALTER migrations. The optimizer schema is not yet deployed to any MySQL environment, so a drop-and-recreate is fine and avoids shipping ALTERs for a brand-new table. - Rename stampExpiryBackoff -> recordExpiryBackoff and replace the "stamp" verbiage in comments, test names, and variables with "write"/"record" so the wording no longer leaks into identifiers. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… create
Reverts the fold into the base create migration. Per Flyway's guidance and this
repo's docs/development/optimizer-schema-migrations.md ("Never edit a migration
that has shipped: Flyway compares every applied migration's checksum at startup
and refuses to start on a mismatch. Fix it with a new migration instead"), a new
column must be added with a new ALTER migration, since V20261008021832 has
shipped and may already be applied to a permanent environment.
- Restore V20261008021832__create_optimizer_tables.sql to its shipped form.
- Add V20261105221200__add_attempts.sql and V20261105221500__add_available_at.sql
(the attempts column keeps the renamed name from Mike's review).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
Make per-table retry timing durable and correct by introducing two operation-row columns — available_at (the authoritative "earliest next schedule" time) and attempts_since_success (the persisted attempt counter). Together they fix two retry-timing bugs where a failed operation would retry at the wrong time, and move the backoff magnitude off a best-effort history window onto a counter carried on the row.
Changes
✓ Bug Fixes
• Reported failure no longer waits out the stale-timeout. On every reported completion, the service now stamps available_at from the analyzer's cadence policy (SUCCESS → success cadence, FAILED → failure backoff). Previously a reported failure sat until the multi-hour active-op stale-timeout elapsed before retrying.
• Dead/missed jobs now back off instead of rescheduling immediately. When the analyzer observes a submitted op that crossed the stale-timeout without ever reporting an outcome, it stamps the failure backoff on the row ( stampExpiryBackoff ) and defers that pass, instead of immediately re-issuing work.
✓ New Features
• available_at TIMESTAMP(6) NULL on table_operations : the authoritative retry gate. CadencePolicy.shouldSchedule is now layered — when available_at is set it is authoritative; otherwise it falls back to the existing active-op + history-cadence logic.
• attempts_since_success on table_operations : attempt number since the last success (reset to 1 on success, carried forward + 1 otherwise), maintained independently of the history windows so a recorded success can't be miscounted. This counter drives the failure-backoff magnitude via nextAvailableAt(from, succeeded, attempts) .
✓ Internal API Changes
• OperationAnalyzer / CadencePolicy gain nextAvailableAt(from, succeeded, attemptsSinceSuccess) ; AnalyzerRunner exposes a nextAvailableAt(operationType, ...) lookup so the completion callback reuses analyzer cadence config (no duplication).
✓ Tests (see below)
Two additive Flyway migrations (new files, never editing a shipped migration):
• V20261105221200__add_attempts_since_success.sql
• V20261105221500__add_available_at.sql
Issue] Briefly discuss the summary of the changes made in this
pull request in 2-3 lines.
For all the boxes checked, please include additional details of the changes made in this pull request.
Testing Done
✓ Added new tests for the changes made.
• CadenceBasedStatsCollectionAnalyzerTest : available_at gate is authoritative (future → blocks even a dead op history would release; past → releases within the stale window); nextAvailableAt applies success cadence vs. failure backoff.
• AnalyzerRunnerTest : timed-out op without available_at is stamped-and-deferred; an already-stamped op is not re-stamped (counter still feeds the gate).
• OptimizerDataServiceImplTest : updateOperation stamps available_at on SUCCESS (success cadence) and FAILED (failure backoff), forwarding the op's attempts_since_success ; leaves it null when no analyzer handles the type.
✓ Updated existing tests to reflect the changes made. The former "logical timeout counted as non-success" runner test is reframed around the new expiry-write behavior.
• All run against Testcontainers MySQL. :libs:optimizer:analyzer:test :libs:optimizer:optimizer-common:build :services:optimizer:test → BUILD SUCCESSFUL.
For all the boxes checked, include a detailed description of the testing done for the changes made in this pull request.
Additional Information
✓ Large PR broken into smaller PRs. This is the retry-timing-state slice of the per-table retry-backoff work (follows PR #812). Follow-ups: attempt-generation + atomic conditional completion/expiry to close writer races (reviewer comments #3/#4), and heartbeat-based ownership renewal.
• No breaking changes: both columns are nullable/additive; available_at == null falls back to existing behavior, so pre-migration rows and in-flight ops are unaffected.
For all the boxes checked, include additional details of the changes made in this pull request.