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.

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

CMDCT-3960: creating new dynamo table for measures with PKs that can be queried instead of scan all #2431

Merged
merged 24 commits into from
Sep 24, 2024

Conversation

angelaco11
Copy link
Contributor

@angelaco11 angelaco11 commented Sep 13, 2024

Description

The current dynamo measures table has a limitation in its primary key structure. It currently is {state}{year}{coreSet}{measure}. The limitation here is the fact that the measure is tacked on to the primary key which makes each primary key unique. Therefore, when we want to do a query to get all the measures for a state, year and coreset, we cannot do a query on the primary key (since it contains each individual measure) and must do a scanAll which is not performant and is causing db threshold alarms.

The changes here are to write a script that creates a new measures table that is rekeyed. The new primary key is {state}{year}{coreSet} and the range/sort key is the measure name. this allows us to query for all measures for a state, year and coreset since all of those measures will have the same primary key (e.g. all measures from AL, 2024 and CCSM will have the primary key AL2024CCSM. The sort/range key will be the measure so that single measures can be retrieved by combining the primary key and the range key. This will make querying the db much more performant and we will not have to use scanAll.

I also updated all calls to query the measures table to point to the new table with the correct keys.

Related ticket(s)

CMDCT-3960


How to test

https://d2flx34i0vw50w.cloudfront.net/

  1. Log in as any user
  2. Click on any coreset and make sure that the measures list loads properly

Notes


Pre-review checklist

  • I have added thorough tests, if necessary
  • I have updated relevant documentation, if necessary
  • I have performed a self-review of my code
  • I have manually tested this PR in the deployed cloud environment

Pre-merge checklist

Review

  • Design: This work has been reviewed and approved by design, if necessary
  • Product: This work has been reviewed and approved by product owner, if necessary

Security

If either of the following are true, notify the team's ISSO (Information System Security Officer).

  • These changes are significant enough to require an update to the SIA.
  • These changes are significant enough to require a penetration test.

convert to a different template: test → val | val → prod

@angelaco11 angelaco11 changed the title [DO NOT REVIEW YET]: CMDCT-3960 CMDCT-3960: creating new dynamo table for measures with PKs that can be queried instead of scan all Sep 17, 2024
@angelaco11 angelaco11 marked this pull request as ready for review September 18, 2024 17:01
@ajaitasaini ajaitasaini self-requested a review September 20, 2024 17:53
Copy link
Contributor

@ajaitasaini ajaitasaini left a comment

Choose a reason for hiding this comment

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

thank you for the thorough description + all the context provided -- made it so easy to understand why we're making these changes! testing via cloudfront worked as expected and the code makes sense / nothing major is sticking out to me

Copy link
Contributor

@benmartin-coforma benmartin-coforma left a comment

Choose a reason for hiding this comment

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

Looks good to me!

services/app-api/handlers/measures/get.ts Show resolved Hide resolved
@benmartin-coforma
Copy link
Contributor

@ailZhou Note that we will need to perform the migration in this PR immediately when it merges into each environment, or we risk data loss.

Copy link

codeclimate bot commented Sep 23, 2024

Code Climate has analyzed commit b01c160 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (90% is the threshold).

This pull request will bring the total coverage in the repository to 84.1% (-0.1% change).

View more on Code Climate.

Copy link
Contributor

@rocio-desantiago rocio-desantiago left a comment

Choose a reason for hiding this comment

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

Code looks good 👍
Tested with deploy link: Measures showing up for each year as expected ✅

Copy link
Collaborator

@ailZhou ailZhou left a comment

Choose a reason for hiding this comment

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

lgtm! I ran it on deploy and i read through the code to the best of my abilities and nothing seemed weird to me.

Thank you for taking on this huge task of migrating the database and increasing QMR's performance!

@ailZhou ailZhou merged commit 366cd75 into master Sep 24, 2024
22 of 23 checks passed
@ailZhou ailZhou deleted the cmdct-3960 branch September 24, 2024 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants