-
Notifications
You must be signed in to change notification settings - Fork 18
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
Conversation
Signed-off-by: Sam Barker <[email protected]>
Signed-off-by: Sam Barker <[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]>
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]>
Co-authored-by: Robert Young <[email protected]> Signed-off-by: Sam Barker <[email protected]>
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.
I think we need to update the README to mention the new metric.
src/main/java/io/strimzi/kafka/quotas/CachingVolumeObserver.java
Outdated
Show resolved
Hide resolved
src/main/java/io/strimzi/kafka/quotas/CachingVolumeObserver.java
Outdated
Show resolved
Hide resolved
import static org.mockito.Mockito.verify; | ||
|
||
@ExtendWith(MockitoExtension.class) | ||
class CachingVolumeObserverTest { |
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.
Nice thorough test.
Signed-off-by: Sam Barker <[email protected]>
Also document why we get it and ignore it during `createCacheKey` Signed-off-by: Sam Barker <[email protected]>
Signed-off-by: Sam Barker <[email protected]>
Signed-off-by: Sam Barker <[email protected]>
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.
LGTM, thanks Sam and Rob
Discussed on the Community call on 19.10.2023: @ppatierno will have a look and review it. |
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.
LGTM.
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