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

[Metadata] Let entries expire in the metadata caches #14154

Merged

Conversation

lhotari
Copy link
Member

@lhotari lhotari commented Feb 7, 2022

Motivation

  • There's essentially a memory leak in the Metadata caches since the entries never expire in the current solution.
  • refreshAfterWrite will never expire entries. After the given time, the next request to the entry will
    trigger a refresh in the background. The current entry will be used until the entry has been refreshed.

Modifications

  • expire after CACHE_REFRESH_TIME_MILLIS * 2

- refreshAfterWrite will never expire entries. After the given time, the next request to the entry will
  trigger a refresh in the background. The current entry will be used until the entry has been refreshed.
  - documentation for Caffeine's refresh feature: https://github.com/ben-manes/caffeine/wiki/Refresh
@github-actions
Copy link

github-actions bot commented Feb 7, 2022

@lhotari:Thanks for your contribution. For this PR, do we need to update docs?
(The PR template contains info about doc, which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)

1 similar comment
@github-actions
Copy link

github-actions bot commented Feb 7, 2022

@lhotari:Thanks for your contribution. For this PR, do we need to update docs?
(The PR template contains info about doc, which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)

@lhotari lhotari added the doc-not-needed Your PR changes do not impact docs label Feb 7, 2022
@github-actions
Copy link

github-actions bot commented Feb 7, 2022

@lhotari:Thanks for providing doc info!

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

This change makes sense to me.

It will also allow to remove more quickly stale data probably

Copy link
Contributor

@Jason918 Jason918 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 it's better not to increase metadata load, how about

  • refresh after CACHE_REFRESH_TIME_MILLIS
  • expire after CACHE_REFRESH_TIME_MILLIS * 2

@lhotari
Copy link
Member Author

lhotari commented Feb 8, 2022

I think it's better not to increase metadata load, how about

  • refresh after CACHE_REFRESH_TIME_MILLIS
  • expire after CACHE_REFRESH_TIME_MILLIS * 2

@Jason918 thanks, I changed it in this way. PTAL

@codelipenghui codelipenghui merged commit 9165aed into apache:master Feb 8, 2022
Copy link
Member

@michaeljmarshall michaeljmarshall left a comment

Choose a reason for hiding this comment

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

LGTM

@codelipenghui codelipenghui added this to the 2.10.0 milestone Feb 21, 2022
Nicklee007 pushed a commit to Nicklee007/pulsar that referenced this pull request Apr 20, 2022
* [Metadata] Let entries expire in the metadata caches

- refreshAfterWrite will never expire entries. After the given time, the next request to the entry will
  trigger a refresh in the background. The current entry will be used until the entry has been refreshed.
  - documentation for Caffeine's refresh feature: https://github.com/ben-manes/caffeine/wiki/Refresh

* Address review comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-not-needed Your PR changes do not impact docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants