chore: migrate ScheduledJob from rethinkdb to pg #9490

jordanh · 2024-03-01T21:47:46Z

Description

Resolves: #9489

I had a bit of extra time last night and decided to try and tackle this small migration. I wanted to see firsthand what kind of effort these migrations (even the simple ones) take.

Testing scenarios

Set a meeting timer / let it elapse
Check scheduled team limits (not checked, as this is not currently in use)

Final checklist

I checked the code review guidelines
I have added Metrics Representative as reviewer(s) if my PR invovles metrics/data/analytics related changes
I have performed a self-review of my code, the same way I'd do it for any other team member
I have tested all cases I listed in the testing scenarios and I haven't found any issues or regressions
Whenever I took a non-obvious choice I added a comment explaining why I did it this way
I added the label Skip Maintainer Review Indicating the PR only requires reviewer review and can be merged right after it's approved if the PR introduces only minor changes, does not contain any architectural changes or does not introduce any new patterns and I think one review is sufficient'
PR title is human readable and could be used in changelog

jordanh · 2024-03-01T21:49:03Z

packages/server/database/types/ScheduledJob.ts

 export type ScheduledJobType =
  | 'MEETING_STAGE_TIME_LIMIT_END'
  | 'LOCK_ORGANIZATION'
  | 'WARN_ORGANIZATION'

 export default abstract class ScheduledJob {
-  id = generateUID()


Postgres now manages the id

jordanh · 2024-03-01T21:50:50Z

packages/server/graphql/mutations/helpers/removeScheduledJobs.ts

+  if (filter) {
+    Object.keys(filter).forEach((key) => {
+      const value = filter[key as keyof FilterType]
+      if (value) query = query.where(key as keyof FilterType, '=', value)


This will "AND ${field} = ${value}" for anything specified in the filter object

jordanh · 2024-03-01T21:51:07Z

packages/server/graphql/mutations/helpers/removeScheduledJobs.ts

-const removeScheduledJobs = async (runAt: Date, filter: {[key: string]: any}) => {
-  const r = await getRethink()
-  return r.table('ScheduledJob').getAll(runAt, {index: 'runAt'}).filter(filter).delete().run()
+type FilterType = Omit<Updateable<DB['ScheduledJob']>, 'runAt'>


Using Updatable makes all the fields optional

jordanh · 2024-03-01T21:57:24Z

packages/server/postgres/migrations/1709312768000_scheduledJobMove.ts

+    }
+  }
+
+  const getNextData = async (leftBoundCursor: Date | undefined) => {


Note: see

parabol/packages/server/postgres/migrations/1677272969994_meetingTemplatesMove.ts

Line 37 in ba7d724

const getNextData = async (leftBoundCursor: Date | undefined) => {

for where I pilfered this logic

mattkrick

code looks good! haven't run a test on it, but can't see any flaws in it from here

mattkrick · 2024-03-01T22:39:19Z

packages/server/billing/helpers/removeTeamsLimitObjects.ts

 import {r} from 'rethinkdb-ts'
 import {RValue} from '../../database/stricterR'
 import {DataLoaderWorker} from '../../graphql/graphql'
 import updateNotification from '../../graphql/public/mutations/helpers/updateNotification'

 const removeTeamsLimitObjects = async (orgId: string, dataLoader: DataLoaderWorker) => {
-  const removeJobTypes = ['LOCK_ORGANIZATION', 'WARN_ORGANIZATION']
+  const removeJobTypes = ['LOCK_ORGANIZATION', 'WARN_ORGANIZATION'] as DB['ScheduledJob']['type'][]


+1 as const is a little narrower here, same can be applied to the line below it or any literal tuple

I keep forgetting this!

jordanh · 2024-03-02T03:27:32Z

@Dschoordsch, @mattkrick had a look – I yield to you if you want to test and merge

Dschoordsch

did not test yet

Dschoordsch · 2024-03-04T19:29:11Z

packages/server/postgres/migrations/1709312768000_moveScheduledJob.ts

+        'batchSize is smaller than the number of items that share the same cursor. Increase batchSize'
+      )
+    }
+    return nextBatch.slice(0, lastMatchingRunAt)


-1 we should include lastMatchingRunAt

return nextBatch.slice(0, lastMatchingRunAt + 1)

otherwise we might lose data. Assume a sequence [..., 4, 5, 5, 6], we would return [..., 4, 5] and start with the open (5, ...) next time, omitting this one element.

This sparks the idea that the migration should read the count first and verify it matches in the count in pg

in future migrations I think I'll probably create a compound index that includes the timestamp + ID so we're guaranteed a unique ID that is also serial so items can still get updated while the migration is running

@Dschoordsch this is not correct, see: #9502 (comment)

Dschoordsch · 2024-03-04T19:54:54Z

packages/server/graphql/private/mutations/runScheduledJobs.ts

+  const upcomingJobs = await pg
+    .selectFrom('ScheduledJob')
+    .selectAll()
+    .where('runAt', '>=', new Date(0))


+1 I think this line could just be removed

Yeah, that's probably true. I was being a bit too direct in my translation ;)

Dschoordsch · 2024-03-04T19:58:31Z

packages/server/postgres/migrations/1709306178000_addScheduledJob.ts

+
+    CREATE INDEX IF NOT EXISTS "idx_ScheduledJob_orgId" ON "ScheduledJob"("orgId");
+    CREATE INDEX IF NOT EXISTS "idx_ScheduledJob_runAt" ON "ScheduledJob"("runAt");
+    CREATE INDEX IF NOT EXISTS "idx_ScheduledJob_type" ON "ScheduledJob"("type");


+1 Not sure we need an index here, it's only used after selecting for orgId, so only a few handful of rows

All of these indexes existed on the rethinkdb table...maybe we keep them for that reason?

- also: removed unecessary transform from earlier revision - also: fixed off-by-1 error in move migration

jordanh · 2024-03-08T20:03:45Z

@Dschoordsch branch has been updated to address one of your code review comments & rebased to master

#9500 should be merged first in order for the migration order to be correct

also: * rebased to master and updated migration order

github-actions bot requested a review from tianrunhe March 1, 2024 21:47

jordanh requested review from Dschoordsch and removed request for tianrunhe March 1, 2024 21:47

github-actions bot added the size/m label Mar 1, 2024

jordanh commented Mar 1, 2024

View reviewed changes

jordanh force-pushed the feat/9489/migrate-scheduledjob branch from 1f3abb7 to 774e867 Compare March 1, 2024 21:55

jordanh commented Mar 1, 2024

View reviewed changes

jordanh force-pushed the feat/9489/migrate-scheduledjob branch from 774e867 to 4cd113e Compare March 1, 2024 21:59

mattkrick approved these changes Mar 1, 2024

View reviewed changes

jordanh force-pushed the feat/9489/migrate-scheduledjob branch 3 times, most recently from d58e5ea to df6024b Compare March 3, 2024 17:53

Dschoordsch requested changes Mar 4, 2024

View reviewed changes

jordanh added 3 commits March 8, 2024 11:55

chore: migrate ScheduledJob from rethinkdb to pg

fbb993f

chore: msk code review update

ac412c2

chore: better name for ScheduledJob migration

23133b0

- also: removed unecessary transform from earlier revision - also: fixed off-by-1 error in move migration

jordanh force-pushed the feat/9489/migrate-scheduledjob branch from df6024b to 6ea106a Compare March 8, 2024 20:01

chore: code review update

a1fc4e9

also: * rebased to master and updated migration order

jordanh force-pushed the feat/9489/migrate-scheduledjob branch from 6ea106a to a1fc4e9 Compare March 8, 2024 20:17

Dschoordsch self-requested a review March 14, 2024 17:29

Dschoordsch approved these changes Mar 14, 2024

View reviewed changes

jordanh merged commit 5c39fde into master Mar 14, 2024
5 checks passed

jordanh deleted the feat/9489/migrate-scheduledjob branch March 14, 2024 20:14

parabol-release-bot bot mentioned this pull request Mar 14, 2024

chore(release): release v7.22.1 #9535

Merged

github-actions bot mentioned this pull request Mar 14, 2024

chore(release): Test v7.22.1 #9538

Closed

24 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: migrate ScheduledJob from rethinkdb to pg #9490

chore: migrate ScheduledJob from rethinkdb to pg #9490

jordanh commented Mar 1, 2024 •

edited

Loading

jordanh Mar 1, 2024

jordanh Mar 1, 2024

jordanh Mar 1, 2024

jordanh Mar 1, 2024

mattkrick left a comment

mattkrick Mar 1, 2024

jordanh Mar 2, 2024

jordanh commented Mar 2, 2024

Dschoordsch left a comment

Dschoordsch Mar 4, 2024

mattkrick Mar 5, 2024

jordanh Mar 8, 2024

Dschoordsch Mar 4, 2024

jordanh Mar 8, 2024

Dschoordsch Mar 4, 2024

jordanh Mar 8, 2024

jordanh commented Mar 8, 2024

chore: migrate ScheduledJob from rethinkdb to pg #9490

chore: migrate ScheduledJob from rethinkdb to pg #9490

Conversation

jordanh commented Mar 1, 2024 • edited Loading

Description

Testing scenarios

Final checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattkrick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jordanh commented Mar 2, 2024

Dschoordsch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jordanh commented Mar 8, 2024

jordanh commented Mar 1, 2024 •

edited

Loading