[BACK-4528] Retire the summary task runners and the outdated-flag mechanism - #969
[BACK-4528] Retire the summary task runners and the outdated-flag mechanism#969toddkazakov wants to merge 4 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (4)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
be999df to
78492bd
Compare
6f9ac80 to
f986282
Compare
78492bd to
5cd5257
Compare
f986282 to
0a9631d
Compare
21e3867 to
1521883
Compare
915c6cf to
163e277
Compare
1521883 to
0e86095
Compare
|
/deploy dev |
|
toddkazakov updated values.yaml file in dev1 |
|
toddkazakov updated flux policies file in dev1 |
|
toddkazakov deployed platform upload-postprocess-cleanup branch to dev1 namespace |
0e86095 to
a60a9f1
Compare
db993f4 to
5b20992
Compare
a60a9f1 to
22452e0
Compare
toddkazakov
changed the title
ewollesen
left a comment
ewollesen
left a comment
There was a problem hiding this comment.
Claude also pointed out that some metrics mentioned in the README are no longer collected. It would be good to update that.
Nothing major, but things that I feel like should be addressed.
Nice work!
| userSummary.SetOutdated(types.OutdatedReasonSchemaMigration) | ||
| userSummary.Dates.Reset() | ||
| // A summary calculated with an outdated schema is recreated from nothing | ||
| *userSummary = *types.Create[PP, PB](userId) |
There was a problem hiding this comment.
Claude points out that the existing summary's _id is lost. Create doesn't set one. Later on, the summary is upserted via userId/type, which means that the existing database record will be updated and still have the correct _id, however the summary in Go won't have the correct _id, it'll have 0.
I didn't follow the whole chain of calls to see if/how userSummary's _id is used from there, but it goes through quite a few other methods, so having an invalid _id seems like a time bomb waiting to explode.
Does it make sense to make sure that the existing userSummary's _id is still there? That's how the old code would have worked I believe, since it didn't recreate from scratch.
This might not have been caught, because the tests that covered this branch were removed.
|
|
||
| func UpdateSummary[PP types.PeriodsPt[P, PB, B], PB types.BucketDataPt[B], P types.Periods, B types.BucketData](dataServiceContext dataService.Context) { | ||
| // UpdateSummary reports the data of the user as changed rather than recalculating synchronously, | ||
| // which the retired task runners required. The work created recalculates every summary of the user. |
There was a problem hiding this comment.
Is there a migration plan to collapsed the three endpoints down to one, since all three do the same work?
3 participants
Single cleanup PR collapsing the previously planned PR2 + PR3 + the legacy field removal, stacked on #968.
Task service: deletes
summary/task(update + migration runners), deregisters them, and replaces theEnsureSummary*Taskdefaults withDeleteDeprecatedTasks, which removes theorg.tidepool.summary.*task rows on startup (kept at least one release).Data service: removes the
GET /v1/summaries/{outdated,migratable}/*routes and handlers.POST /v1/summaries/{type}/{userId}remains as a manual recalc lever: it now enqueues upload postprocess work (DATA_ADDED) and returns 202 instead of recalculating synchronously. ShrinksSummarizer(dropsSetOutdated/GetOutdatedUserIDs/GetMigratableUserIDs), removes the corresponding store methods, thetidepool_summary_queue_lag/queue_lengthmetrics, the data-client methods and panic stubs, and the two outdated index models (physical index drops stay in the ops runbook).Legacy fields: removes
Dates.OutdatedSince/OutdatedReason/LastUpdatedReason, theOutdatedReason*constants,SetOutdated/SetNotOutdated, andtypes.OutdatedSummariesResponse. Grep clearance: blip references the fields only in a Storybook fixture; jellyfish only as local variable names; clinic service carries them in its own API where they simply go empty. A schema-version mismatch now recreates the summary from nothing (previouslySetOutdated(SCHEMA_MIGRATION)+ reset), and the no-new-data path no longer writes (there is no flag left to clear).Deploy gates: PR #968 and jellyfish deployed everywhere;
tidepool_summary_queue_lengthat zero and staying there; deploy together with tidepool-org/clinic-worker#123, which removes the CDC EHR-sync trigger that readlastUpdatedReason.Tests: 82 packages green across
summary/...,data/...,task/...,work/...; the failing-cursor regression test is ported fromGetOutdatedUserIDstoListMigratableUserIDs.