Skip to content

file: load from file should allow configure maxBytes#86

Merged
makasim merged 1 commit intomasterfrom
load-from-file-expose-max-bytes
May 21, 2025
Merged

file: load from file should allow configure maxBytes#86
makasim merged 1 commit intomasterfrom
load-from-file-expose-max-bytes

Conversation

@makasim
Copy link
Member

@makasim makasim commented May 21, 2025

It is needed to implement same behavior as LoadFromFileOrNew does. It respects given maxBytes and pass it to load so this check

fastcache/file.go

Lines 133 to 139 in e439f07

if maxBytes > 0 {
maxBucketBytes := uint64((maxBytes + bucketsCount - 1) / bucketsCount)
expectedBucketChunks := (maxBucketBytes + chunkSize - 1) / chunkSize
if maxBucketChunks != expectedBucketChunks {
return nil, fmt.Errorf("cache file %s contains maxBytes=%d; want %d", filePath, maxBytes, expectedBucketChunks*chunkSize*bucketsCount)
}
}
works.

Related to https://github.com/VictoriaMetrics/VictoriaMetrics/pull/8952/files

@makasim makasim requested review from hagen1778 and tenmozes May 21, 2025 13:21
@codecov
Copy link

codecov bot commented May 21, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.96%. Comparing base (e439f07) to head (4905d51).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master      #86   +/-   ##
=======================================
  Coverage   76.96%   76.96%           
=======================================
  Files           4        4           
  Lines         547      547           
=======================================
  Hits          421      421           
  Misses         71       71           
  Partials       55       55           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@makasim makasim marked this pull request as ready for review May 21, 2025 13:26
@makasim makasim requested a review from zekker6 May 21, 2025 16:02
@makasim makasim merged commit 198c85e into master May 21, 2025
7 checks passed
@makasim makasim deleted the load-from-file-expose-max-bytes branch May 21, 2025 16:16
@valyala
Copy link
Collaborator

valyala commented Jun 2, 2025

@makasim , @zekker6 .

This pull request breaks users' code, since it changes the signature of the public function - LoadFromFile() - from LoadFromFile(filePath string) to LoadFromFile(filePath string, maxBytes int). This is a big no-go for public Go packages - see https://go.dev/doc/modules/release-workflow#breaking . Please return back the signature of the LoadFromFile() function to the previous one, add a new public function, which accepts maxBytes argument and make a new release, which fixes the breaking change.

Note that this rule must be applied to all our public packages, which have v1.* release (e.g. stable release).

@makasim
Copy link
Member Author

makasim commented Jun 2, 2025

@valyala Apologies for this, it's entirely my fault. I'll get it fixed right away.

@makasim
Copy link
Member Author

makasim commented Jun 3, 2025

reverted in #88

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.

3 participants