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

Add caching of volume observations to mitigate inconsistencies between API calls. #42

Merged
merged 13 commits into from
Oct 26, 2023

Conversation

SamBarker
Copy link

This PR adds the notion of an observation time to the Volume Usage data and a CachingVolumeObserver which uses that to ensure we maintain valid observations for the configured validity duration.

The caching is used to address issues where one set of brokers is returned by the describe cluster API call, however one of those brokers goes offline and is not included in the results of the describe logDirs API call.

This is a limited to solution as it relies on in memory state and thus there remain scenarios where re-starting a broker can lead to problematic throttling decisions. The additional overhead of maintaining shared state between plug-ins in the cluster combined with how unlikely this case is to occur suggests its not worth the effort of addressing that.

Fixes: #40

SamBarker and others added 9 commits September 18, 2023 11:23
Signed-off-by: Sam Barker <[email protected]>
Co-authored-by: Robert Young <[email protected]>
Signed-off-by: Sam Barker <[email protected]>
Co-authored-by: Robert Young <[email protected]>
Signed-off-by: Sam Barker <[email protected]>
Co-authored-by: Robert Young <[email protected]>
Co-authored-by: Robert Young <[email protected]>
Signed-off-by: Sam Barker <[email protected]>
Co-authored-by: Robert Young <[email protected]>
Signed-off-by: Sam Barker <[email protected]>
@scholzj scholzj added this to the 0.3.0 milestone Sep 25, 2023
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.

I think we need to update the README to mention the new metric.

import static org.mockito.Mockito.verify;

@ExtendWith(MockitoExtension.class)
class CachingVolumeObserverTest {
Copy link
Member

Choose a reason for hiding this comment

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

Nice thorough test.

@SamBarker SamBarker mentioned this pull request Oct 2, 2023
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.

LGTM, thanks Sam and Rob

@scholzj
Copy link
Member

scholzj commented Oct 19, 2023

Discussed on the Community call on 19.10.2023: @ppatierno will have a look and review it.

Copy link
Member

@ppatierno ppatierno left a comment

Choose a reason for hiding this comment

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

LGTM.

@ppatierno ppatierno merged commit 08197ea into strimzi:main Oct 26, 2023
@SamBarker SamBarker deleted the activeSetInconsistency branch November 19, 2024 22:44
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.

Broker triggering throttling being restarted.
4 participants