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

feat: add logic for image reference counting #609

Open
wants to merge 3 commits into
base: staging
Choose a base branch
from

Conversation

carissa-tang
Copy link
Collaborator

@carissa-tang carissa-tang commented Oct 30, 2023

Notion ticket link

Add Reference Counting to Images

Implementation description

  • Add ImageCount model, interface, implementation, and unit tests
  • Update ImageUploadService
    • Initialize count on image creation
    • Decrement count on delete, and delete image only if count is 0
    • Add incrementImageCount function for test service
    • Increment count on upload of existing image (i.e. filePath exists)
  • Update TestService
    • Increment image count on duplicate test

Steps to test

  1. Create a test with an image. Check that the reference count is 1 and the image is uploaded to GCP.
    image

  2. Duplicate a test. Check that the reference count is 2. Check that the same image is still in GCP and there is no new upload.
    image

  3. Delete the duplicate test. Check that the reference count is 1. Check that the same image is still in GCP.
    image

  4. Delete the original test. Check that the record is deleted from the DB. Check that the image is deleted from GCP.

What should reviewers focus on?

Checklist

  • My PR name is descriptive and in imperative tense
  • My commit messages are descriptive and in imperative tense. My commits are atomic and trivial commits are squashed or fixup'd into non-trivial commits
  • I have run the appropriate linter(s)
  • I have requested a review from the PL, as well as other devs who have background knowledge on this PR or who will be building on top of this PR

@carissa-tang carissa-tang changed the title add logic for reference counting feat: add logic for image reference counting Oct 30, 2023
backend/services/implementations/testService.ts Outdated Show resolved Hide resolved
Comment on lines +413 to +416
await this.processImages<ImageMetadata>(
questions,
this.imageUploadService.incrementImageCount.bind(this.imageUploadService),
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this runs for all images, have you done any sort of load test before/after? There's probably minimal difference considering things should theoretically run in parallel, but it would be good to verify

@jfdoming jfdoming added question Further information is requested requested changes and removed ready for review question Further information is requested labels Nov 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants