Support dynamic update cache config #13679

lordcheng10 · 2022-01-08T15:53:26Z

Motivation

Support dynamic update cache config :

managedLedgerCacheEvictionTimeThresholdMillis
managedLedgerCacheSizeMB
managedLedgerCacheEvictionWatermark

Documentation

Check the box below or label this PR directly (if you have committer privilege).

Need to update docs?

doc-required

(If you need help on updating docs, create a doc issue)
no-need-doc

(Please explain why)
doc

(If this PR contains doc changes)

lordcheng10 · 2022-01-08T16:02:59Z

@hangc0276 @michaeljmarshall @merlimat @shoothzj @eolivelli PTAL,thanks!

lordcheng10 · 2022-01-08T16:57:58Z

/pulsarbot run-failure-checks

lordcheng10 · 2022-01-08T19:09:11Z

/pulsarbot run-failure-checks

managed-ledger/pom.xml

lordcheng10 · 2022-01-09T03:21:37Z

/pulsarbot run-failure-checks

lordcheng10 · 2022-01-09T03:47:20Z

/pulsarbot run-failure-checks

lordcheng10 · 2022-01-09T05:12:12Z

/pulsarbot run-failure-checks

eolivelli

A general comment about this implementation.

Referring everything to the Service Configuration is a waste of resource and we should avoid this especially inside synchonized methods.

I suggest to not evaluate the values of the configuration parameters everything but to try to notify the components that the Configuration changed and to update the current local configuration when such notification happens

managed-ledger/pom.xml

lordcheng10 · 2022-01-10T05:08:58Z

/pulsarbot run-failure-checks

lordcheng10 · 2022-01-10T07:02:26Z

/pulsarbot run-failure-checks

lordcheng10 · 2022-01-10T11:14:22Z

/pulsarbot run-failure-checks

lordcheng10 · 2022-01-10T14:40:56Z

/pulsarbot run-failure-checks

lordcheng10 · 2022-01-11T05:33:35Z

/pulsarbot run-failure-checks

lordcheng10 · 2022-01-11T08:39:08Z

/pulsarbot run-failure-checks

lordcheng10 · 2022-01-11T13:45:41Z

/pulsarbot run-failure-checks

lordcheng10 · 2022-01-11T13:46:20Z

A general comment about this implementation.

Referring everything to the Service Configuration is a waste of resource and we should avoid this especially inside synchonized methods.

I suggest to not evaluate the values of the configuration parameters everything but to try to notify the components that the Configuration changed and to update the current local configuration when such notification happens

OK I will fix

github-actions · 2022-02-26T01:48:38Z

The pr had no activity for 30 days, mark with Stale label.

lordcheng10 · 2022-03-13T06:44:25Z

Fixed.
PTAL,thanks! @eolivelli @Jason918 @codelipenghui

…Impl and add updateCacheEvictionTimeThreshold for cacheEvictionTimeThresholdNanos

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/ManagedLedgerFactory.java

aloyszhang

LGTM

lordcheng10 · 2022-03-14T09:44:13Z

We should also add tests

Fixed.
PTAL,thanks! @eolivelli

eolivelli

we are on our way.
I left some minor feedback, last corners to be rounded

eolivelli · 2022-03-14T11:22:05Z

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/ManagedLedgerFactory.java

+    /**
+     * @return time threshold for eviction.
+     * */
+    long getCacheEvictionTimeThresholdNanos();


please give a name consistent with updateCacheEvictionTimeThreshold (I would remove "nanos")

Fixed.
PTAL,thanks! @eolivelli

eolivelli · 2022-03-14T11:22:46Z

pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApiTest.java

+        admin.brokers().updateDynamicConfiguration("managedLedgerCacheEvictionTimeThresholdMillis", "2000");
+
+        // wait config to be updated
+        for (int i = 0; i < 5; i++) {


why only "5" ?

please consider using Awaiatility

Fixed.
PTAL,thanks! @eolivelli

eolivelli

LGTM

awesome work

michaeljmarshall

@lordcheng10 - are you able to provide motivation for this PR?

michaeljmarshall · 2022-03-14T17:39:45Z

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/EntryCacheManager.java

+    private volatile long maxSize;
+    private volatile long evictionTriggerThreshold;
+    private volatile double cacheEvictionWatermark;


I am concerned about the cost of this change. These three variables are read each time that a message is written to the cache. While I see the generic benefit of increasing configurability, I don't think we should do so in a way that adds cost (even small costs) to the write path. @lordcheng10 - do you have a use case that shows why the benefits out way the costs here?

@merlimat - do you have any thoughts here? I remember that you have raised concerns about volatile in the metadatastore.

I know that we are talking about minor optimizations here, but since this is on the write path, I feel it makes sense to discuss now.

do you have a use case that shows why the benefits out way the costs here?

If we need to adjust the cache parameters, we need to restart the online cluster in turn. Restarting the cluster will have a certain impact on online business.

For different business scenarios, we cannot give a more reasonable cache configuration at once. May need to adjust the cache configuration multiple times

I am concerned about the cost of this change. These three variables are read each time that a message is written to the cache. While I see the generic benefit of increasing configurability, I don't think we should do so in a way that adds cost (even small costs) to the write path.

Do you have some good ideas? Regarding the need to provide a dynamic configuration cache, but also to solve the problems you mentioned.

@lordcheng10 - thanks for the additional context. It would be interesting to see a flame graph for the write path of the broker to identify hot spots. The only reason this PR caught my attention is because of the general heuristic that I've seen @merlimat employ: avoid volatile when possible. Without actual metrics about performance impact, I think we should leave this PR in place.

(cherry picked from commit b0213b2)

Squash merge branch 'cache_config' into '2.8.1' Fixes #<xyz> ### Motivation --story=872772869 支持cache动态配置(apache#13679) TAPD: --story=872772869

(cherry picked from commit b0213b2)

Squash merge branch 'cache_config' into '2.8.1' Fixes #<xyz> ### Motivation --story=872772869 支持cache动态配置(apache#13679) TAPD: --story=872772869

(cherry picked from commit b0213b2)

github-actions bot assigned lordcheng10 Jan 8, 2022

github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jan 8, 2022

lordcheng10 changed the title ~~support dynamic update config : managedLedgerCacheEvictionTimeThresh…~~ Support dynamic update cache config Jan 8, 2022

Jason918 reviewed Jan 9, 2022

View reviewed changes

managed-ledger/pom.xml Outdated Show resolved Hide resolved

eolivelli requested changes Jan 9, 2022

View reviewed changes

managed-ledger/pom.xml Outdated Show resolved Hide resolved

codelipenghui modified the milestones: 2.11.0, 2.10.0 Jan 18, 2022

github-actions bot added the lifecycle/stale label Feb 26, 2022

use registerConfigurationListener for dynamic update cache configs

a541c4c

lordcheng10 force-pushed the support_dynamic_cache_config branch from 2a75405 to a541c4c Compare March 13, 2022 06:43

check style

515d3f2

github-actions bot removed the lifecycle/stale label Mar 14, 2022

lordcheng10 added 4 commits March 14, 2022 11:28

move updateCacheSizeAndThreshold into method in ManagedLedgerFactory…

5e7e36d

…Impl and add updateCacheEvictionTimeThreshold for cacheEvictionTimeThresholdNanos

check style

bc80b0e

check style

0eb2ee2

check style

8954714

lordcheng10 requested a review from eolivelli March 14, 2022 04:29

aloyszhang reviewed Mar 14, 2022

View reviewed changes

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/ManagedLedgerFactory.java Outdated Show resolved Hide resolved

remove updateCacheSizeAndThreshold

091fd88

lordcheng10 requested a review from aloyszhang March 14, 2022 04:39

aloyszhang approved these changes Mar 14, 2022

View reviewed changes

add unit test

c5c8a1f

eolivelli requested changes Mar 14, 2022

View reviewed changes

lordcheng10 added 2 commits March 14, 2022 19:56

1.rename getCacheEvictionTimeThresholdNanos;2.use Awaiatility

a27d889

check style

c34cf73

lordcheng10 requested a review from eolivelli March 14, 2022 11:57

eolivelli approved these changes Mar 14, 2022

View reviewed changes

eolivelli merged commit b0213b2 into apache:master Mar 14, 2022

michaeljmarshall reviewed Mar 14, 2022

View reviewed changes

Nicklee007 pushed a commit to Nicklee007/pulsar that referenced this pull request Apr 20, 2022

Support dynamic update cache config (apache#13679)

f05c2f4

nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Jul 8, 2022

Support dynamic update cache config (apache#13679)

8a6f0fb

(cherry picked from commit b0213b2)

congbobo184 added cherry-picked/branch-2.9 Archived: 2.9 is end of life release/2.9.4 labels Nov 17, 2022

congbobo184 pushed a commit that referenced this pull request Nov 17, 2022

Support dynamic update cache config (#13679)

5093c07

(cherry picked from commit b0213b2)

congbobo184 pushed a commit that referenced this pull request Dec 7, 2022

Support dynamic update cache config (#13679)

4caf3ef

(cherry picked from commit b0213b2)

liangyepianzhou mentioned this pull request Dec 19, 2022

[cherry-pick][branch-2.10]Fix broker cache eviction of entries read by active cursor #18980

Merged

14 tasks

liangyepianzhou added cherry-picked/branch-2.10 release/2.10.3 labels Dec 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support dynamic update cache config #13679

Support dynamic update cache config #13679

lordcheng10 commented Jan 8, 2022 •

edited

lordcheng10 commented Jan 8, 2022

lordcheng10 commented Jan 8, 2022

lordcheng10 commented Jan 8, 2022

lordcheng10 commented Jan 9, 2022

lordcheng10 commented Jan 9, 2022

lordcheng10 commented Jan 9, 2022

eolivelli left a comment

lordcheng10 commented Jan 10, 2022

lordcheng10 commented Jan 10, 2022

lordcheng10 commented Jan 10, 2022

lordcheng10 commented Jan 10, 2022

lordcheng10 commented Jan 11, 2022

lordcheng10 commented Jan 11, 2022

lordcheng10 commented Jan 11, 2022

lordcheng10 commented Jan 11, 2022 •

edited

github-actions bot commented Feb 26, 2022

lordcheng10 commented Mar 13, 2022 •

edited

aloyszhang left a comment

lordcheng10 commented Mar 14, 2022 •

edited

eolivelli left a comment

eolivelli Mar 14, 2022

lordcheng10 Mar 14, 2022

eolivelli Mar 14, 2022

lordcheng10 Mar 14, 2022

eolivelli left a comment

michaeljmarshall left a comment

michaeljmarshall Mar 14, 2022

michaeljmarshall Mar 14, 2022

lordcheng10 Mar 15, 2022 •

edited

lordcheng10 Mar 15, 2022 •

edited

michaeljmarshall Mar 16, 2022

Support dynamic update cache config #13679

Support dynamic update cache config #13679

Conversation

lordcheng10 commented Jan 8, 2022 • edited

Motivation

Documentation

lordcheng10 commented Jan 8, 2022

lordcheng10 commented Jan 8, 2022

lordcheng10 commented Jan 8, 2022

lordcheng10 commented Jan 9, 2022

lordcheng10 commented Jan 9, 2022

lordcheng10 commented Jan 9, 2022

eolivelli left a comment

Choose a reason for hiding this comment

lordcheng10 commented Jan 10, 2022

lordcheng10 commented Jan 10, 2022

lordcheng10 commented Jan 10, 2022

lordcheng10 commented Jan 10, 2022

lordcheng10 commented Jan 11, 2022

lordcheng10 commented Jan 11, 2022

lordcheng10 commented Jan 11, 2022

lordcheng10 commented Jan 11, 2022 • edited

github-actions bot commented Feb 26, 2022

lordcheng10 commented Mar 13, 2022 • edited

aloyszhang left a comment

Choose a reason for hiding this comment

lordcheng10 commented Mar 14, 2022 • edited

eolivelli left a comment

Choose a reason for hiding this comment

eolivelli Mar 14, 2022

Choose a reason for hiding this comment

lordcheng10 Mar 14, 2022

Choose a reason for hiding this comment

eolivelli Mar 14, 2022

Choose a reason for hiding this comment

lordcheng10 Mar 14, 2022

Choose a reason for hiding this comment

eolivelli left a comment

Choose a reason for hiding this comment

michaeljmarshall left a comment

Choose a reason for hiding this comment

michaeljmarshall Mar 14, 2022

Choose a reason for hiding this comment

michaeljmarshall Mar 14, 2022

Choose a reason for hiding this comment

lordcheng10 Mar 15, 2022 • edited

Choose a reason for hiding this comment

lordcheng10 Mar 15, 2022 • edited

Choose a reason for hiding this comment

michaeljmarshall Mar 16, 2022

Choose a reason for hiding this comment

lordcheng10 commented Jan 8, 2022 •

edited

lordcheng10 commented Jan 11, 2022 •

edited

lordcheng10 commented Mar 13, 2022 •

edited

lordcheng10 commented Mar 14, 2022 •

edited

lordcheng10 Mar 15, 2022 •

edited

lordcheng10 Mar 15, 2022 •

edited