Update ValueOverflowThreshold#1799
Merged
Merged
Conversation
308d386 to
ff97fab
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR changes ValueOverflowThreshold from an integer byte count to a memory-size string, centralizes byte-level validation in GarnetServerOptions, and updates related configuration defaults/help text and tests.
Changes:
- Parses and validates
ValueOverflowThresholdas a memory-size string with bounds and page-fit checks. - Adds a 512-byte minimum for main-log page size.
- Updates defaults/help text and adds configuration parsing/validation tests.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
libs/server/Servers/GarnetServerOptions.cs |
Converts threshold handling to string parsing and validation before passing to Tsavorite settings. |
libs/server/Servers/ServerOptions.cs |
Adds a minimum effective main-log page size check. |
libs/host/Configuration/Options.cs |
Updates CLI option type/help text for threshold and page settings. |
libs/host/Configuration/OptionsValidators.cs |
Tightens memory-size regex validation. |
libs/host/defaults.conf |
Updates default threshold value and config comments. |
test/Garnet.test/GarnetServerConfigTests.cs |
Adds tests for threshold parsing, validation, page-fit checks, and page minimums. |
kevin-montrose
approved these changes
May 13, 2026
tiagonapoli
approved these changes
May 13, 2026
0e18d1d to
3508402
Compare
tiagonapoli
approved these changes
May 13, 2026
3508402 to
e9d7057
Compare
…ze minimum
ValueOverflowThreshold (Options and GarnetServerOptions) changes from int to a
memory-size string (e.g. '4k', '1m'); default is '4k'.
All byte-level validation lives in GarnetServerOptions.ValueOverflowThresholdBytes()
(single source of truth, no attribute-level duplication):
* Parsed via ServerOptions.TryParseSize.
* Range enforced as [64 B, 256 MB], matching Tsavorite's MaxInlineValueSize bounds
(1 << LogSettings.kLowestMaxInlineSizeBits .. 1 << (LogSettings.kMaxStringSizeBits - 1)).
* Cross-property check: the value (after rounding down to previous power of 2)
must be strictly less than the (rounded) PageSize, so an inline value of this
size plus per-record overhead can fit on a page. If the cross-check is not
satisfied, the value is clamped down to PageSize/2 with a warning (rather
than failing) to remain consistent with the silent power-of-2 rounding used
by other size settings.
ServerOptions enforces a minimum page size of 512 bytes (MinPageSizeBytes constant)
via a shared ValidatedPageSizeBits(value, propName) helper. This helper is used by
both PageSizeBits() (for the main log) and ReadCachePageSizeBits() (called from
GarnetServerOptions.GetSettings() for the read-cache log), so a sub-512 B
ReadCachePageSize is now rejected the same way as a sub-512 B PageSize.
At Garnet's defaults (MaxInlineKeySize = 128 B Tsavorite default, single-byte
namespace), the worst-case inline record at PageSize = 512 B with the cross-check
applied (effective MaxInlineValueSize <= 256 B) plus the 64 B page header is
~490 B; 512 B is the smallest power-of-2 page size that accommodates this and
prevents Tsavorite's 'Entry does not fit on page' exception.
Help text on PageSize and ReadCachePageSize updated to mention the 512 B minimum.
Side fix: tighten MemorySizeValidationAttribute regex from
'^\d+([K|k|M|m|G|g][B|b]{0,1})?$' to '^\d+([KkMmGg][Bb]?)?$'.
The old form treated '|' as a literal inside the character class and accepted
strings like '4|'.
defaults.conf updated: ValueOverflowThreshold value '4096' -> '"4k"', and
PageSize / ReadCachePageSize comments note the 512 B minimum.
Tests added in GarnetServerConfigTests:
* ValueOverflowThresholdParsing (default + valid CLI/JSON values).
* ValueOverflowThresholdValidation (invalid format + out-of-range rejected at
server-options consumption time; also covers the '4|' regex regression).
* ValueOverflowThresholdMustFitOnPage (cross-check vs PageSize; verifies clamp).
* MinimumPageSize (PageSize below 512 B rejected).
* MinimumReadCachePageSize (ReadCachePageSize below 512 B rejected when
read-cache is enabled).
Breaking change: existing JSON configs that used a numeric ValueOverflowThreshold
(e.g. 4096) must switch to a string ('4k' or '4096').
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
e9d7057 to
83c6bf8
Compare
TedHartMS
approved these changes
May 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ValueOverflowThreshold (Options and GarnetServerOptions) changes from int to a memory-size string (e.g. '4k', '1m'); default is '4k'.
All byte-level validation lives in GarnetServerOptions.ValueOverflowThresholdBytes() (single source of truth, no attribute-level duplication):
ServerOptions.PageSizeBits() enforces a minimum PageSize of 512 bytes (MinPageSizeBytes constant). At Garnet's defaults (MaxInlineKeySize = 128 B Tsavorite default, single-byte namespace), the worst-case inline record at PageSize = 512 B with the cross-check applied (effective MaxInlineValueSize <= 256 B) plus the 64 B page header is ~490 B; 512 B is the smallest power-of-2 page size that accommodates this and prevents Tsavorite's 'Entry does not fit on page' exception.
Help text on PageSize and ReadCachePageSize updated to mention the 512 B minimum.
Side fix: tighten MemorySizeValidationAttribute regex from
'^\d+([K|k|M|m|G|g][B|b]{0,1})?$' to '^\d+([KkMmGg][Bb]?)?$'.
The old form treated '|' as a literal inside the character class and accepted strings like '4|'.
defaults.conf updated: ValueOverflowThreshold value '4096' -> '"4k"', and PageSize / ReadCachePageSize comments note the 512 B minimum.
Tests added in GarnetServerConfigTests:
Breaking change: existing JSON configs that used a numeric ValueOverflowThreshold (e.g. 4096) must switch to a string ('4k' or '4096').