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

storage: apply limits to segment.bytes #6492

Merged
merged 2 commits into from
Sep 26, 2022

Conversation

jcsp
Copy link
Contributor

@jcsp jcsp commented Sep 21, 2022

Cover letter

To avoid introducing incompatibilities with applications that have existing segment.bytes settings during topic creation, we do not refuse topic creations/alterations that violate these limits. Instead, we accept the user input, but clamp it when applying the segment size limit, so that the effective segment size remains within the bounds even if the apparent segment.bytes property violates it.

Fixes: #6036

Backport Required

  • not a bug fix
  • issue does not exist in previous branches
  • papercut/not impactful enough to backport
  • v22.2.x
  • v22.1.x
  • v21.11.x

UX changes

See release notes.

Release notes

Features

  • Tunable cluster configuration properties are added to set bounds on the segment.bytes topic property. If log_segment_size_min and/or log_segment_size_max are set, then any segment.bytes outside these bounds will be silently clamped to the permitted range. This prevents poorly-chosen configurations from inducing the cluster to create very large numbers of small segment files, or extremely large segment files.

dotnwat
dotnwat previously approved these changes Sep 21, 2022
Copy link
Member

@mmaslankaprv mmaslankaprv left a comment

Choose a reason for hiding this comment

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

Should we also validate this while setting topic properties in AlterConfiguration ?

@jcsp
Copy link
Contributor Author

jcsp commented Sep 21, 2022

Should we also validate this while setting topic properties in AlterConfiguration ?

The thinking behind the silent clamping is:

  • someone might have an existing app/script/tool that expects to be able to set segment.bytes arbitrarily, and I would rather quietly give them a different segment size than have it perceived a Kafka incompatibility ("Redpanda doesn't work with my app").
  • be able to apply the limits retroactively on systems where someone has perhaps already chosen bad segment sizes.

For self-hosted I think explicitly validating inputs would be fine (they can always change the limit if they don't like it), but in the cloud these limits can't be changed by the user.

We could always add the input validation later on top of this change if needed.

@mmaslankaprv
Copy link
Member

Should we also validate this while setting topic properties in AlterConfiguration ?

The thinking behind the silent clamping is:

  • someone might have an existing app/script/tool that expects to be able to set segment.bytes arbitrarily, and I would rather quietly give them a different segment size than have it perceived a Kafka incompatibility ("Redpanda doesn't work with my app").
  • be able to apply the limits retroactively on systems where someone has perhaps already chosen bad segment sizes.

For self-hosted I think explicitly validating inputs would be fine (they can always change the limit if they don't like it), but in the cloud these limits can't be changed by the user.

We could always add the input validation later on top of this change if needed.

This makes sense to me.

mmaslankaprv
mmaslankaprv previously approved these changes Sep 22, 2022
@jcsp
Copy link
Contributor Author

jcsp commented Sep 22, 2022

Force pushed to fix the new test (it wasn't clearing the properties at end of test, which caused other tests to misbehave)

Twiddling global configs in tests is a bit ugly, but these configs don't belong in the per-log config object. When we do #6431, these should probably move into storage_resources and make that the central place for deciding what the effective segment size limit should be for a partition (this will be dynamic based on available disk space).

@jcsp
Copy link
Contributor Author

jcsp commented Sep 23, 2022

The test needed updating again because I wasn't taking account of segment size jitter (nothing is ever simple!)

This is the second time that jitter has bitten me when trying to write a test, so I've also made a small refactor to disable jitter in the storage unit tests by default.

...and disable it in unit tests.

This is not a user facing change.  For users they
still get the 5% default: it's only in unit tests
that we disable it to enable deterministic tests.
To avoid introducing incompatibilities with applications
that have existing segment.bytes settings during topic
creation, we do not refuse topic creations/alterations
that violate these limits.  Instead, we accept the user input,
but clamp it when applying the segment size limit, so that
the effective segment size remains within the bounds even
if the apparent segment.bytes property violates it.

Fixes: redpanda-data#6036
@jcsp jcsp merged commit f0358b7 into redpanda-data:dev Sep 26, 2022
@jcsp jcsp deleted the segment-size-bounds branch September 26, 2022 11:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Configurable bounds on segment sizes
4 participants