Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

chore: migrate Invoice to pg #9502

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

chore: migrate Invoice to pg #9502

wants to merge 1 commit into from

Conversation

jordanh
Copy link
Contributor

@jordanh jordanh commented Mar 3, 2024

Description

Resolves #9501

Demo

[If possible, please include a screenshot or gif/video, it'll make it easier for reviewers to understand the scope of the changes and how the change is supposed to work. If you're introducing something new or changing the existing patterns, please share a Loom and explain what decisions you've made and under what circumstances]

Testing scenarios

[Please list all the testing scenarios a reviewer has to check before approving the PR]

  • Scenario A

    • Step 1
    • Step 2...

  • Scenario B

    • Step 1
    • Step 2....

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

@github-actions github-actions bot added the size/m label Mar 3, 2024
jordanh
jordanh commented Mar 3, 2024
View reviewed changes
packages/server/postgres/migrations/1709478683000_moveInvoice.ts Outdated
'batchSize is smaller than the number of items that share the same cursor. Increase batchSize'
)
}
return nextBatch.slice(0, lastMatchingStartAt)
Copy link
Contributor Author

@jordanh jordanh Mar 3, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found a tricky off-by-one bug here from this function based on parabol/packages/server/postgres/migrations/1677272969994_meetingTemplatesMove.ts

Turned up after I migrated the Invoice data, I noticed I was missing exactly 1 row.

I wrote quick code to export both tables to CSV and then wrote a quick Python script to compare them. Yielded the missing row:

grep Hk9-U-VcZ *.csv
pgdata.csv:"2017-09-11 12:48:02+00","Hk9-U-VcZ"
rethinkdata.csv:"2017-09-11T12:48:02.000Z","Hk9-U-VcZ"
rethinkdata.csv:"2017-09-11T12:48:02.000Z","Hk9-U-VcZ"

Created a conditional breakpoint in this function to find when this org was being processed and it helped me see that this function's logic works by using the last index key as the left open interval...we were inadvertently only every going to get 1 row for any duplicate key that fit within batchSize

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did some testing and turns out you're right, shame on me.

@jordanh jordanh force-pushed the chore/9501/migrate-invoice branch 2 times, most recently from d9cf21a to da34442 Compare March 4, 2024 07:58
This was referenced Mar 8, 2024
Merged
Merged
@jordanh
chore: migrate Invoice to pg
a3301c1 
   - also:
      - bumps kysely-codegen for proper JSON typing
        (see: RobinBlomberg/kysely-codegen#124)
      - fixes off-by-one error in parabol/packages/server/postgres/migrations/1677272969994_meetingTemplatesMove.ts
        to avoid copying a bad pattern – I wonder if this should become
	a helper function instead?
@jordanh jordanh force-pushed the chore/9501/migrate-invoice branch from da34442 to a3301c1 Compare March 8, 2024 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Dschoordsch Dschoordsch Dschoordsch left review comments

Assignees
No one assigned
Labels
size/m
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

chore: migrate Invoice to pg (1/1)
2 participants
@jordanh @Dschoordsch