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.

Sign up for GitHub

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

Jump to bottom

Add min available bytes storage policy #33

Merged
merged 50 commits into from
Feb 15, 2023
Merged

Add min available bytes storage policy #33

merged 50 commits into from
Feb 15, 2023

Conversation

SamBarker
Copy link

First step in implementing proposal #47.

This PR adds the first of the new limit types "min available bytes" and adds the requirement to specify the bootstrap URL for communicating with the Kafka cluster. It also removes the old storage checker, but preserves the old total consumed space soft and hard limits (as an incremental step we plan to remove them eventually) as well as their metrics.

We propose to add additional metrics and other limit types in separate pull requests.

@SamBarker SamBarker mentioned this pull request Jan 31, 2023
Closed
SamBarker and others added 16 commits January 31, 2023 21:00
@SamBarker @robobario
Refactor storage checker to push volume details instead of total stor…
ecdcf71 
…age.

Co-authored-by: Robert Young <robeyoun@redhat.com>
Signed-off-by: Sam Barker <sbarker@redhat.com>
@SamBarker @robobario
Remove redundant tracking of used storage
7cdd8d1 
Co-authored-by: Robert Young <robeyoun@redhat.com>
Signed-off-by: Sam Barker <sbarker@redhat.com>
@SamBarker
Use mockitoExtension
de470a8 
Signed-off-by: Sam Barker <sbarker@redhat.com>
@SamBarker @robobario
Introduce ClusterVolumeSource
f4b080a 
Co-authored-by: Robert Young <robeyoun@redhat.com>
Signed-off-by: Sam Barker <sbarker@redhat.com>
@robobario @SamBarker
Add more tests to cover multiple volumes per broker and multiple brokers
5cbecfc 
Co-authored-by: Sam Barker <sbarker@redhat.com>
Signed-off-by: Sam Barker <sbarker@redhat.com>
@SamBarker @robobario
Extract a VolumeSourceBuilder to wire up different volume sources
0ce61e3 
Co-authored-by: Robert Young <robeyoun@redhat.com>
Signed-off-by: Sam Barker <sbarker@redhat.com>
@SamBarker @robobario
Update to use explicit adminClient configuration inline with proposal.
664c783 
Co-authored-by: Robert Young <robeyoun@redhat.com>
Signed-off-by: Sam Barker <sbarker@redhat.com>
@robobario @SamBarker
Add TotalConsumedThrottleFactorSupplier
40115db 
Signed-off-by: Sam Barker <sbarker@redhat.com>
@SamBarker
Pull Consumer<Collection<Volume>> up onto the ThrottleFactorSupplier.
2f73836 
Wire up quota reset listener.
Co-authored-by: Robert Young <robeyoun@redhat.com>

Signed-off-by: Sam Barker <sbarker@redhat.com>
@SamBarker @robobario
Move limit metrics to TotalConsumedThrottleFactorSupplier
8a516d0 
Co-authored-by: Robert Young <robeyoun@redhat.com>
Signed-off-by: Sam Barker <sbarker@redhat.com>
@SamBarker @robobario
Throttle on available bytes.
095ffa6 
Co-authored-by: Robert Young <robeyoun@redhat.com>
Signed-off-by: Sam Barker <sbarker@redhat.com>
@SamBarker @robobario
Configure available bytes limit.
3ac3142 
Co-authored-by: Robert Young <robeyoun@redhat.com>
Signed-off-by: Sam Barker <sbarker@redhat.com>
@SamBarker @robobario
Add validation to available bytes limit argument
0c6ee52 
Co-authored-by: Robert Young <robeyoun@redhat.com>
Signed-off-by: Sam Barker <sbarker@redhat.com>
@SamBarker @robobario
Stop using local volume source aka StorageChecker.
78e5387 
Co-authored-by: Robert Young <robeyoun@redhat.com>
Signed-off-by: Sam Barker <sbarker@redhat.com>
@SamBarker @robobario
RIP StorageChecker
cf53a29 
Co-authored-by: Robert Young <robeyoun@redhat.com>
Signed-off-by: Sam Barker <sbarker@redhat.com>
@SamBarker @robobario
Make admin bootstrap servers required.
9a752ae 
Co-authored-by: Robert Young <robeyoun@redhat.com>
Signed-off-by: Sam Barker <sbarker@redhat.com>
@scholzj scholzj added this to the 0.3.0 milestone Jan 31, 2023
@scholzj
Copy link
Member

scholzj commented Jan 31, 2023

Looks like some spotbugs issues failed the build.

@SamBarker
Add Javadoc to make the JDK 17 build happy.
fc5babe 
Signed-off-by: Sam Barker <sbarker@redhat.com>
@SamBarker
Switch AvailableBytesThrottleFactorSupplier to use a boolean to track…
b3cc1b6 
… throttled state.

This stops spotbugs complaining about floating point equality.

Signed-off-by: Sam Barker <sbarker@redhat.com>
@SamBarker
Add StaticLongGauge to keep spotbugs happy.
27c6713 
Signed-off-by: Sam Barker <sbarker@redhat.com>
@SamBarker
Suppress findbugs false positive warnings.
dfc12ab 
Signed-off-by: Sam Barker <sbarker@redhat.com>
SamBarker and others added 3 commits February 13, 2023 12:17
@SamBarker @robobario
Javadoc for new interfaces.
edd3997 
Co-authored-by: Robert Young <robeyoun@redhat.com>
Signed-off-by: Sam Barker <sbarker@redhat.com>
@SamBarker @robobario
Remove dead code.
c4ac2aa 
We can re-introduce filterable log dirs later.

Co-authored-by: Robert Young <robeyoun@redhat.com>
Signed-off-by: Sam Barker <sbarker@redhat.com>
@SamBarker @robobario
spotbugs is still wrong!
533c78f 
Co-authored-by: Robert Young <robeyoun@redhat.com>
Signed-off-by: Sam Barker <sbarker@redhat.com>
@SamBarker
Copy link
Author

@tombentley: @robobario and I think we have addressed all your feedback hopefully the changes work for you.

(cc: @scholzj if you are still planning to look at the java code)

tombentley
tombentley approved these changes Feb 13, 2023
View reviewed changes
Copy link
Member

@tombentley tombentley left a comment

Choose a reason for hiding this comment

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

A few more nits, but assuming you're happy to fix those this LGTM. Thanks!

src/main/java/io/strimzi/kafka/quotas/StaticQuotaConfig.java Outdated Show resolved Hide resolved
src/main/java/io/strimzi/kafka/quotas/VolumeUsage.java Show resolved Hide resolved
src/main/java/io/strimzi/kafka/quotas/AvailableBytesThrottleFactorPolicy.java Show resolved Hide resolved
src/main/java/io/strimzi/kafka/quotas/PolicyBasedThrottle.java Outdated Show resolved Hide resolved
src/main/java/io/strimzi/kafka/quotas/PolicyBasedThrottle.java Outdated Show resolved Hide resolved
src/main/java/io/strimzi/kafka/quotas/TotalConsumedThrottleFactorPolicy.java Outdated Show resolved Hide resolved
src/test/java/io/strimzi/kafka/quotas/AvailableBytesThrottleFactorPolicyTest.java Show resolved Hide resolved
src/test/java/io/strimzi/kafka/quotas/AvailableBytesThrottleFactorPolicyTest.java Outdated Show resolved Hide resolved
SamBarker and others added 5 commits February 14, 2023 09:47
@SamBarker @robobario
pr nit: Double -> double.
ed052f6 
Co-authored-by: Robert Young <robeyoun@redhat.com>
Signed-off-by: Sam Barker <sbarker@redhat.com>
@SamBarker @robobario
pr nit: Fix capitalisation of description
c36fa43 
Co-authored-by: Robert Young <robeyoun@redhat.com>
Signed-off-by: Sam Barker <sbarker@redhat.com>
@SamBarker @robobario
pr nit: extract constant
79a180c 
Co-authored-by: Robert Young <robeyoun@redhat.com>
Signed-off-by: Sam Barker <sbarker@redhat.com>
@SamBarker @robobario
Optimize soft throttle calculation.
103181e 
Co-authored-by: Robert Young <robeyoun@redhat.com>
Signed-off-by: Sam Barker <sbarker@redhat.com>
@SamBarker @robobario
Filter out Volumes for which the broker doesn't report usage values.
fa5bfa3 
Co-authored-by: Robert Young <robeyoun@redhat.com>
Signed-off-by: Sam Barker <sbarker@redhat.com>
scholzj
scholzj approved these changes Feb 14, 2023
View reviewed changes
Copy link
Member

@scholzj scholzj left a comment

Choose a reason for hiding this comment

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

LGTM I guess ... assuming it works :-D

@scholzj
Copy link
Member

scholzj commented Feb 14, 2023

@ppatierno @strimzi/maintainers Anyone has anything else to this?

@robobario @SamBarker
Add logging on threshold changes
99f664d 
Why:
We will want visibility when the throttle factor changes as it could
effectively stop messages being produced (and make recovery visible too)

Co-authored-by: Sam Barker <sbarker@redhat.com>
Signed-off-by: Robert Young <robeyoun@redhat.com>
@scholzj scholzj merged commit ea44ab3 into strimzi:main Feb 15, 2023
robobario added a commit to robobario/kafka-quotas-plugin that referenced this pull request Feb 15, 2023
@robobario
Remove deprecated total disk usage soft/hard limit types
279e75b 
Why:
Acting on the total usage of the cluster makes no sense now that we are
using KIP-827 to determine the usage of all volumes in the active broker
set. Nodes will enter/exit the active set as a matter of course. So the
aggregate usage will fluctuate. Instead we will limit based on the usage
of each volume going forward. IE throttle if any of the volumes is getting
too full.

See Also:
* https://github.com/strimzi/proposals/blob/main/047-cluster-wide-volume-usage-quota-management.md
* [Add min available bytes storage policy](strimzi#33)
robobario added a commit to robobario/kafka-quotas-plugin that referenced this pull request Feb 15, 2023
@robobario
Remove deprecated total disk usage soft/hard limit types
b5cff42 
Why:
Acting on the total usage of the cluster makes no sense now that we are
using KIP-827 to determine the usage of all volumes in the active broker
set. Nodes will enter/exit the active set as a matter of course. So the
aggregate usage will fluctuate. Instead we will limit based on the usage
of each volume going forward. IE throttle if any of the volumes is getting
too full.

See Also:
* https://github.com/strimzi/proposals/blob/main/047-cluster-wide-volume-usage-quota-management.md
* [Add min available bytes storage policy](strimzi#33)
robobario added a commit to robobario/kafka-quotas-plugin that referenced this pull request Feb 15, 2023
@robobario
Remove deprecated total disk usage soft/hard limit types
4c29f01 
Why:
Acting on the total usage of the cluster makes no sense now that we are
using KIP-827 to determine the usage of all volumes in the active broker
set. Nodes will enter/exit the active set as a matter of course. So the
aggregate usage will fluctuate. Instead we will limit based on the usage
of each volume going forward. IE throttle if any of the volumes is getting
too full.

See Also:
* https://github.com/strimzi/proposals/blob/main/047-cluster-wide-volume-usage-quota-management.md
* [Add min available bytes storage policy](strimzi#33)
robobario added a commit to robobario/kafka-quotas-plugin that referenced this pull request Feb 15, 2023
@robobario
Add available ratio limit type
c13b013 
Why:
It may be convenient to be able to throttle message production when any volume's
usage ratio drops below a threshold. This means you could have a single configuration
for heterogeneous disks saying cease all message production if any volume's available
ratio is below 0.01

See Also:
* https://github.com/strimzi/proposals/blob/main/047-cluster-wide-volume-usage-quota-management.md
* [Add min available bytes storage policy](strimzi#33)

Signed-off-by: Robert Young <robeyoun@redhat.com>
robobario added a commit to robobario/kafka-quotas-plugin that referenced this pull request Feb 15, 2023
@robobario
Add available ratio limit type
465ba1c 
Why:
It may be convenient to be able to throttle message production when any volume's
usage ratio drops below a threshold. This means you could have a single configuration
for heterogeneous disks saying cease all message production if any volume's available
ratio is below 0.01

See Also:
* https://github.com/strimzi/proposals/blob/main/047-cluster-wide-volume-usage-quota-management.md
* [Add min available bytes storage policy](strimzi#33)

Signed-off-by: Robert Young <robeyoun@redhat.com>
@robobario robobario mentioned this pull request Feb 15, 2023
Merged
robobario added a commit to robobario/kafka-quotas-plugin that referenced this pull request Feb 15, 2023
@robobario
Remove deprecated total disk usage soft/hard limit types
45ee870 
Why:
Acting on the total usage of the cluster makes no sense now that we are
using KIP-827 to determine the usage of all volumes in the active broker
set. Nodes will enter/exit the active set as a matter of course. So the
aggregate usage will fluctuate. Instead we will limit based on the usage
of each volume going forward. IE throttle if any of the volumes is getting
too full.

See Also:
* https://github.com/strimzi/proposals/blob/main/047-cluster-wide-volume-usage-quota-management.md
* [Add min available bytes storage policy](strimzi#33)

Signed-off-by: Robert Young <robeyoun@redhat.com>
robobario added a commit to robobario/kafka-quotas-plugin that referenced this pull request Feb 15, 2023
@robobario
Add available ratio limit type
a849c18 
Why:
It may be convenient to be able to throttle message production when any volume's
usage ratio drops below a threshold. This means you could have a single configuration
for heterogeneous disks saying cease all message production if any volume's available
ratio is below 0.01

See Also:
* https://github.com/strimzi/proposals/blob/main/047-cluster-wide-volume-usage-quota-management.md
* [Add min available bytes storage policy](strimzi#33)

Signed-off-by: Robert Young <robeyoun@redhat.com>
robobario added a commit to robobario/kafka-quotas-plugin that referenced this pull request Feb 15, 2023
@robobario
Add available ratio limit type
78b29e1 
Why:
It may be convenient to be able to throttle message production when any volume's
usage ratio drops below a threshold. This means you could have a single configuration
for heterogeneous disks saying cease all message production if any volume's available
ratio is below 0.01

See Also:
* https://github.com/strimzi/proposals/blob/main/047-cluster-wide-volume-usage-quota-management.md
* [Add min available bytes storage policy](strimzi#33)

Signed-off-by: Robert Young <robeyoun@redhat.com>
robobario added a commit to robobario/kafka-quotas-plugin that referenced this pull request Feb 15, 2023
@robobario
Add available ratio limit type
c397f53 
Enables users to throttle the produce quota when the availableRatio (availableBytes/totalBytes) is
less than or equal to a threshold.

The new configuration property is:
`client.quota.callback.static.storage.per.volume.limit.min.available.ratio`

Which accepts a double between 0 and 1 inclusive

Why:
It may be convenient to be able to throttle message production when any volume's
usage ratio drops below a threshold. This means you could have a single configuration
for heterogeneous disks saying cease all message production if any volume's available
ratio is below 0.01

See Also:
* https://github.com/strimzi/proposals/blob/main/047-cluster-wide-volume-usage-quota-management.md
* [Add min available bytes storage policy](strimzi#33)

Signed-off-by: Robert Young <robeyoun@redhat.com>

refactor: extract a base class for policies applied to any volume

Signed-off-by: Robert Young <robeyoun@redhat.com>
robobario added a commit to robobario/kafka-quotas-plugin that referenced this pull request Feb 28, 2023
@robobario
Add available ratio limit type
d46c274 
Enables users to throttle the produce quota when the availableRatio (availableBytes/totalBytes) is
less than or equal to a threshold.

The new configuration property is:
`client.quota.callback.static.storage.per.volume.limit.min.available.ratio`

Which accepts a double between 0 and 1 inclusive

Why:
It may be convenient to be able to throttle message production when any volume's
usage ratio drops below a threshold. This means you could have a single configuration
for heterogeneous disks saying cease all message production if any volume's available
ratio is below 0.01

See Also:
* https://github.com/strimzi/proposals/blob/main/047-cluster-wide-volume-usage-quota-management.md
* [Add min available bytes storage policy](strimzi#33)

Signed-off-by: Robert Young <robeyoun@redhat.com>

refactor: extract a base class for policies applied to any volume

Signed-off-by: Robert Young <robeyoun@redhat.com>
@robobario robobario mentioned this pull request Feb 28, 2023
Merged
scholzj pushed a commit that referenced this pull request Mar 1, 2023
@robobario
Remove deprecated total disk usage soft/hard limit types (#34)
db7285c 
* Remove deprecated total disk usage soft/hard limit types

Why:
Acting on the total usage of the cluster makes no sense now that we are
using KIP-827 to determine the usage of all volumes in the active broker
set. Nodes will enter/exit the active set as a matter of course. So the
aggregate usage will fluctuate. Instead we will limit based on the usage
of each volume going forward. IE throttle if any of the volumes is getting
too full.

See Also:
* https://github.com/strimzi/proposals/blob/main/047-cluster-wide-volume-usage-quota-management.md
* [Add min available bytes storage policy](#33)

Signed-off-by: Robert Young <robeyoun@redhat.com>

* Remove Strimzi Usage documentation

Why:
It's unknown precisely how Strimzi will integrate with the new version, and
the docs could be confusing as they contradict the warning about it not being
included in Strimzi yet

Signed-off-by: Robert Young <robeyoun@redhat.com>

---------

Signed-off-by: Robert Young <robeyoun@redhat.com>
robobario added a commit to robobario/kafka-quotas-plugin that referenced this pull request Mar 1, 2023
@robobario
Add available ratio limit type
c1ee697 
Enables users to throttle the produce quota when the availableRatio (availableBytes/totalBytes) is
less than or equal to a threshold.

The new configuration property is:
`client.quota.callback.static.storage.per.volume.limit.min.available.ratio`

Which accepts a double between 0 and 1 inclusive

Why:
It may be convenient to be able to throttle message production when any volume's
usage ratio drops below a threshold. This means you could have a single configuration
for heterogeneous disks saying cease all message production if any volume's available
ratio is below 0.01

See Also:
* https://github.com/strimzi/proposals/blob/main/047-cluster-wide-volume-usage-quota-management.md
* [Add min available bytes storage policy](strimzi#33)

Signed-off-by: Robert Young <robeyoun@redhat.com>

refactor: extract a base class for policies applied to any volume

Signed-off-by: Robert Young <robeyoun@redhat.com>
scholzj pushed a commit that referenced this pull request Mar 2, 2023
@robobario
Add available ratio limit type (#36)
3260111 
Enables users to throttle the produce quota when the availableRatio (availableBytes/totalBytes) is
less than or equal to a threshold.

The new configuration property is:
`client.quota.callback.static.storage.per.volume.limit.min.available.ratio`

Which accepts a double between 0 and 1 inclusive

Why:
It may be convenient to be able to throttle message production when any volume's
usage ratio drops below a threshold. This means you could have a single configuration
for heterogeneous disks saying cease all message production if any volume's available
ratio is below 0.01

See Also:
* https://github.com/strimzi/proposals/blob/main/047-cluster-wide-volume-usage-quota-management.md
* [Add min available bytes storage policy](#33)



refactor: extract a base class for policies applied to any volume

Signed-off-by: Robert Young <robeyoun@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@robobario robobario robobario left review comments

@tombentley tombentley tombentley approved these changes

@scholzj scholzj scholzj approved these changes

@ppatierno ppatierno Awaiting requested review from ppatierno

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.3.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@SamBarker @scholzj @tombentley @robobario