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

Allow to configure the managed ledger cache eviction frequency #4066

Merged
merged 17 commits into from
May 2, 2019

Conversation

merlimat
Copy link
Contributor

Motivation

Currently there's a rate limiter that avoid evicting from cache each time the read position is updated but rather only does that every 1 sec at max. The primary reason is to avoid mutex contention across different cursors trying to trigger the eviction in parallel.

The frequency should be higher though, to make sure we promptly discard all entries once all active cursors have moved past a particular point.

Modifications

  • Allow to configure the frequency from broker.conf
  • Increased default to 100/s

@merlimat merlimat added the type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages label Apr 17, 2019
@merlimat merlimat added this to the 2.4.0 milestone Apr 17, 2019
@merlimat merlimat self-assigned this Apr 17, 2019
Copy link
Contributor

@rdhabalia rdhabalia left a comment

Choose a reason for hiding this comment

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

minor comment regarding test.

// activate caught up cursors
cursors.forEach(cursor -> {
if (cursor.getNumberOfEntries() < maxActiveCursorBacklogEntries) {
if (cursor.getNumberOfEntries() < backloggedCursorThresholdEntries) {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@merlimat
Copy link
Contributor Author

@rdhabalia Updated with an additional time threshold and check for slowest active consumer. Please take another look.

Copy link
Contributor

@rdhabalia rdhabalia left a comment

Choose a reason for hiding this comment

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

LGTM.. few minor comments..

@@ -136,9 +136,6 @@ public synchronized void updateStats(
} catch (Exception e) {
log.error("Failed to generate topic stats for topic {}: {}", name, e.getMessage(), e);
}
// this task: helps to activate inactive-backlog-cursors which have caught up and
// connected, also deactivate active-backlog-cursors which has backlog
((PersistentTopic) topic).getManagedLedger().checkBackloggedCursors();
Copy link
Contributor

Choose a reason for hiding this comment

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

if we have removed from here then who triggers checkBackloggedCursors(..)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

uhm.. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Contributor

@rdhabalia rdhabalia left a comment

Choose a reason for hiding this comment

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

👍

@rdhabalia
Copy link
Contributor

rdhabalia commented Apr 24, 2019

@merlimat we may also want to add config to standalone-config file and site-documentation file.

@merlimat
Copy link
Contributor Author

@merlimat we may also want to add config to config files.

Which one are you referring? I added into broker.conf

@rdhabalia
Copy link
Contributor

Which one are you referring? I added into broker.conf

may be we can add configs to :
https://github.com/apache/pulsar/blob/master/site2/docs/reference-configuration.md
https://github.com/apache/pulsar/blob/master/conf/standalone.conf

@merlimat
Copy link
Contributor Author

Good point, Added.

Copy link
Contributor

@ivankelly ivankelly left a comment

Choose a reason for hiding this comment

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

Is there any testing for the time based eviction? I don't see it

@merlimat
Copy link
Contributor Author

@ivankelly Good point. Added explicit test for time-based eviction.

@merlimat
Copy link
Contributor Author

merlimat commented May 2, 2019

run java8 tests

@merlimat merlimat merged commit f5c7b22 into apache:master May 2, 2019
sijie pushed a commit that referenced this pull request Jan 18, 2020
### Motivation
Some parameters are added in the `broker.conf` and `standalone.conf` files. However, those parameters are not updated in the docs.
See the following PRs for details: #4150, #4066, #4197, #3819, #4261, #4273, #4320.

### Modifications
Add those parameter info, and sync docs with the code.

Does not update the description quite much, there are two reasons for this:
1. Keep doc content consistent with code. We need to update the description for those parameters in the code first, and then sync them in docs.
2. Will adopt a generator to generate those content automatically in the near future.
huangdx0726 pushed a commit to huangdx0726/pulsar that referenced this pull request Aug 24, 2020
### Motivation
Some parameters are added in the `broker.conf` and `standalone.conf` files. However, those parameters are not updated in the docs.
See the following PRs for details: apache#4150, apache#4066, apache#4197, apache#3819, apache#4261, apache#4273, apache#4320.

### Modifications
Add those parameter info, and sync docs with the code.

Does not update the description quite much, there are two reasons for this:
1. Keep doc content consistent with code. We need to update the description for those parameters in the code first, and then sync them in docs.
2. Will adopt a generator to generate those content automatically in the near future.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants