-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
pkg/ccl/storageccl/engineccl: ensure encryption percentage does not e… #103783
pkg/ccl/storageccl/engineccl: ensure encryption percentage does not e… #103783
Conversation
00c16da
to
b6241af
Compare
f69c191
to
431a100
Compare
package: pkg/ccl/storageccl/engineccl Previously, the TotalFiles property from GenEnvStats() returned the total number of files known to Pebble, however this results in encryption percentages that are greater than 100%. The fix is to set TotalFiles to be the maximum of the files known to Pebble and the files known by the file registry. This applies to the TotalBytes property as well. Fixes: cockroachdb#97874 Release note: None
431a100
to
3fa3d2d
Compare
Don't we want to cap it to ActiveFiles? |
Previously, RaduBerinde wrote…
Feels like if there are files unaccounted by Pebble, we would show something under 100% in the steady-state |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @itsbilal and @RaduBerinde)
pkg/storage/pebble.go
line 1877 at r1 (raw file):
Previously, RaduBerinde wrote…
Feels like if there are files unaccounted by Pebble, we would show something under 100% in the steady-state
Since there's a bit of subtlety here, maybe we should add a comment:
len(fr.Files)
is the total number of files known to the file registry. This includes all files created by Cockroach since encryption-at-rest was first enabled. If encryption-at-rest is subsequently disabled, new files will still be encoded within the registry. If encryption-at-rest has always been enabled, this count is as complete as you can get.
The existing stats.TotalFiles
(computed up above from Pebble Metrics
), holds an approximation of all the files known to Pebble. This does not include files that are being asynchronously deleted by Pebble, files that are stored in the auxiliary directory (typically sstables being prepared for ingestion by sideloading), or files created by spilling-to-disk.
If we knew encryption-at-rest had always been enabled, len(fr.Files)
would be the true number of files. Since we don't know that, we use Pebble's count of files as a lower bound.
bors r+ |
This PR was included in a batch that timed out, it will be automatically retried |
Build succeeded: |
ensure encryption percentage does not exceed 100%
package: pkg/ccl/storageccl/engineccl
Previously, the
TotalFiles
property fromGenEnvStats()
returned thetotal number of files known to Pebble, however this results in encryption
percentages that are greater than 100%. The fix is to set TotalFiles to
be the maximum of the files known to Pebble and the files known by the
file registry. This applies to the
TotalBytes
property as well.Fixes: #97874
Release note: None