Skip to content

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
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