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

[39] Implement DELETE /api/cameras/<uuid>/<stream>/view.mp4?s=X-Y to delete whole recordings #323

Closed

Conversation

breel-dev
Copy link

#39

Continuous garbage collection only seems to start from server/db/writer.rs#delete_oldest_recordings. I don't want to delete "enough old recordings to be within disk restrictions" without pruning the API deleted recordings first. I made the LockedDatabase::delete_recordings() also re-initialize the SampleFileDirs' garbage_needs_unlink collections.

While DELETE /api/cameras/.../.../view.mp4 is a little strange, given GET view.mp4 does a live transformation to MP4 to justify its name, I do like the DELETE endpoint is guessable from the GET endpoint.

Copy link
Owner

@scottlamb scottlamb 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! Nice surprise to get this PR. I left a few comments below.

server/db/db.rs Outdated Show resolved Hide resolved
@@ -506,6 +507,38 @@ Bugs and limitations:
`hasTrailingZero` above, and
[#178](https://github.com/scottlamb/moonfire-nvr/issues/178).

### `DELETE /api/cameras/<uuid>/<stream>/view.mp4`
Copy link
Owner

Choose a reason for hiding this comment

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

While DELETE /api/cameras/.../.../view.mp4 is a little strange, given GET view.mp4 does a live transformation to MP4 to justify its name, I do like the DELETE endpoint is guessable from the GET endpoint.

Yeah, that name is a bit strange. The guessability is nice but hopefully not too important when we have these API docs.

My first thought is that maybe this should be a POST to /recordings. We also should take in the csrf parameter for this as we do for other non-GET stuff, and that gives us a place to put it in a similar format, and to attach more preconditions and such if desired. What do you think?

Copy link
Author

Choose a reason for hiding this comment

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

csrf is a good callout. Will update.

Is there a sub-path of POST /api/camera/.../.../recordings/XYZ that could work? Or POST /api/camera/.../.../XYZ?

server/db/db.rs Outdated Show resolved Hide resolved
ref/api.md Outdated Show resolved Hide resolved
ref/api.md Outdated Show resolved Hide resolved
@breel-dev
Copy link
Author

Ready for another pass

@breel-dev
Copy link
Author

Ran into warnings about already deleted files and panics over missing garbage. Have not resolved.

@breel-dev breel-dev closed this Sep 7, 2024
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.

2 participants