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

Renew image time to cleanup on comment Preview and Submit #992

Merged
merged 3 commits into from
May 24, 2021

Conversation

paskal
Copy link
Sponsor Collaborator

@paskal paskal commented May 7, 2021

That PR will delay the image clean up after it's loaded by preview, so the commenter would have more time to post it before cleanup removes it.

Also, reset the cleanup time on images in the submitted comment to prevent the situation when they are deleted from temporary storage before being move to the permanent one, waiting for comment EditDuration to expire. That resolves #909.

@paskal paskal added the backend label May 7, 2021
@paskal paskal requested a review from umputun May 7, 2021 22:43
@paskal paskal force-pushed the dverhoturov/renew_image_on_load branch from 8d28f5e to fd1c47a Compare May 7, 2021 22:48
@coveralls
Copy link

coveralls commented May 7, 2021

Pull Request Test Coverage Report for Build 853908858

  • 26 of 33 (78.79%) changed or added relevant lines in 5 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.03%) to 83.614%

Changes Missing Coverage Covered Lines Changed/Added Lines %
backend/app/store/image/image.go 6 9 66.67%
backend/app/store/image/bolt_store.go 6 10 60.0%
Totals Coverage Status
Change from base Build 851855721: -0.03%
Covered Lines: 5419
Relevant Lines: 6481

💛 - Coveralls

@paskal paskal force-pushed the dverhoturov/renew_image_on_load branch from fd1c47a to 627ec41 Compare May 7, 2021 23:49
@paskal paskal marked this pull request as draft May 7, 2021 23:49
@paskal paskal force-pushed the dverhoturov/renew_image_on_load branch 2 times, most recently from 183758d to 1bfdfab Compare May 8, 2021 00:31
@paskal paskal marked this pull request as ready for review May 8, 2021 00:37
@paskal paskal force-pushed the dverhoturov/renew_image_on_load branch from 1bfdfab to c950d70 Compare May 8, 2021 00:42
@umputun
Copy link
Owner

umputun commented May 8, 2021

I see what you did and this solution can probably work but there are a couple of things I don't like:

  • the idea to make this countdown reset a part of storage feels odd. This is some kind of temporary state and keeping it in the real long-term store gives us only one benefit - the ability to survive reboots and, as a nice side effect, distributed mode (if the store supports it). I don't think we really care about the first one, and for the second - some sort of cache seems to be a closer match
  • The implementation not insanely complex but still too much for what we are trying to achieve here.

as I opened the ticket my idea was to isolate all of it on the image and service levels. i.e. some sort of event bus from rest to image communicating change event. I'm not sure if this even possible as I have not tried to write the actual code, just a feeling. Maybe moving this countdown store closer to the image will do it

@paskal
Copy link
Sponsor Collaborator Author

paskal commented May 8, 2021

If the staging storage is part of the image.Store interface as it is now, we can't alter its commit and cleanup behaviour other than the way I did it above. The only way around is I can think of is implementing separate staging storage for images that will be independent of the normal one, then we'll have commit and cleanup and cleanup timer reset functionality in that separate package and not in three of them as of now.

What do you think?

@paskal paskal marked this pull request as draft May 14, 2021 10:06
@paskal paskal force-pushed the dverhoturov/renew_image_on_load branch 3 times, most recently from bedc8f8 to e40c68d Compare May 16, 2021 21:02
@paskal paskal changed the title Renew image time to cleanup on Load Renew image time to cleanup on comment Preview and Submit May 16, 2021
@paskal paskal force-pushed the dverhoturov/renew_image_on_load branch 2 times, most recently from 5f1dc2c to 32286ad Compare May 16, 2021 23:49
@paskal paskal force-pushed the dverhoturov/renew_image_on_load branch 2 times, most recently from 2966571 to 4dd25aa Compare May 17, 2021 07:00
@paskal paskal marked this pull request as ready for review May 17, 2021 07:01
@codecov
Copy link

codecov bot commented May 17, 2021

Codecov Report

Merging #992 (3211079) into master (cbdf5f1) will increase coverage by 0.01%.
The diff coverage is 92.30%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #992      +/-   ##
==========================================
+ Coverage   43.99%   44.01%   +0.01%     
==========================================
  Files         127      127              
  Lines        2905     2906       +1     
  Branches      656      657       +1     
==========================================
+ Hits         1278     1279       +1     
  Misses       1614     1614              
  Partials       13       13              
Impacted Files Coverage Δ
frontend/app/components/user-info/user-info.tsx 0.00% <0.00%> (ø)
frontend/app/components/avatar/avatar.tsx 100.00% <100.00%> (ø)
frontend/app/components/avatar/index.ts 100.00% <100.00%> (ø)
frontend/app/components/comment/comment.tsx 64.85% <100.00%> (-0.12%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4d011aa...3211079. Read the comment docs.

Also:

- make commitTTL equal to EditDuration,
  so that image is committed to permanent
  storage after comment can no longer be edited
- move cleanupTTL to Cleanup function,
  as it's not used elsewhere in the code
- add variables to some tests sleeps, so that
  instead of being magic numbers they would
  rely on timers of structures they suppose
  to wait for
@paskal paskal force-pushed the dverhoturov/renew_image_on_load branch from 4dd25aa to 3211079 Compare May 18, 2021 15:52
Copy link
Owner

@umputun umputun left a comment

Choose a reason for hiding this comment

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

LGTM

@umputun umputun merged commit 9fa23cc into master May 24, 2021
@umputun umputun deleted the dverhoturov/renew_image_on_load branch May 24, 2021 22:34
@paskal paskal added this to the v1.9 milestone Jan 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Image get cleaned up while waiting for CommitTTL to expire
3 participants