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 mtime as optional ETag algorithm in file backend alongside default md5 #613

Merged
merged 3 commits into from
Jan 14, 2025

Conversation

ciscorn
Copy link
Contributor

@ciscorn ciscorn commented Jan 12, 2025

What kind of change does this PR introduce?

Fix #612

What is the current behavior?

#612

In the file backend, ETags are generated by calculating the MD5 hash of the entire file contents, which can take several seconds or even more than 10 seconds for larger files. This becomes particularly problematic when utilizing Range requests.

What is the new behavior?

The ETag in the file backend is now generated using only a file's modification time (mtime) and size. I believe this is sufficient as it follows the same approach used by Nginx (and moreover, the file backend is primarily used in local development).

@ciscorn ciscorn changed the title fix: Optimize etag generation to improve file backend performance fix: Optimize ETag generation in file backend to remove delays with large files Jan 12, 2025
@fenos
Copy link
Contributor

fenos commented Jan 13, 2025

Hi @ciscorn Awesome optimisation!
Could you please keep this backwards compatible and use an environment variable to control the checksum algorithm please

@ciscorn ciscorn force-pushed the fix-file-backend-etag branch from bbf6a72 to da7aff6 Compare January 14, 2025 06:03
@ciscorn
Copy link
Contributor Author

ciscorn commented Jan 14, 2025

@fenos Thanks!
I’ve modified the implementation to respect the environment variable. While I think “mtime” is a reasonable default, I’ve set “md5” as the default to maintain backward compatibility. What do you think?

src/config.ts Outdated Show resolved Hide resolved
@fenos
Copy link
Contributor

fenos commented Jan 14, 2025

Awesome job @ciscorn
Just one little change requested, to keep the md5 as the default algorithm to not break existing projects.

In a major version we could consider switching to this as default

@ciscorn ciscorn requested a review from fenos January 14, 2025 06:13
@ciscorn ciscorn changed the title fix: Optimize ETag generation in file backend to remove delays with large files feat: Add ‘mtime’ as optional ETag algorithm in file backend alongside default ‘md5’ Jan 14, 2025
@ciscorn ciscorn changed the title feat: Add ‘mtime’ as optional ETag algorithm in file backend alongside default ‘md5’ feat: Add mtime as optional ETag algorithm in file backend alongside default md5 Jan 14, 2025
@fenos fenos merged commit 53cd5d6 into supabase:master Jan 14, 2025
@fenos
Copy link
Contributor

fenos commented Jan 14, 2025

Awesome job @ciscorn 🎉

Copy link

🎉 This PR is included in version 1.17.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@ciscorn ciscorn deleted the fix-file-backend-etag branch January 14, 2025 12:48
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.

File backend is very slow for Range requests to large files
2 participants