Jason918 commented on a change in pull request #14488:
URL: https://github.com/apache/pulsar/pull/14488#discussion_r838176918
##########
File path:
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerFactoryImpl.java
##########
@@ -252,23 +252,13 @@ private synchronized void refreshStats() {
lastStatTimestamp = now;
}
- private void cacheEvictionTask() {
- double evictionFrequency =
Math.max(Math.min(config.getCacheEvictionFrequency(), 1000.0), 0.001);
- long waitTimeMillis = (long) (1000 / evictionFrequency);
-
- while (true) {
- try {
- doCacheEviction();
-
- Thread.sleep(waitTimeMillis);
- } catch (InterruptedException e) {
- // Factory is shutting down
- return;
- } catch (Throwable t) {
- log.warn("Exception while performing cache eviction: {}",
t.getMessage(), t);
- }
+ Runnable cacheEvictionTask = () -> {
Review comment:
It seems we can use just use anonymous lambda in Line200 instead of
making `cacheEvictionTask` a new field,
##########
File path:
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java
##########
@@ -1699,7 +1699,15 @@
private double managedLedgerCacheEvictionWatermark = 0.9;
@FieldContext(category = CATEGORY_STORAGE_ML,
doc = "Configure the cache eviction frequency for the managed
ledger cache. Default is 100/s")
- private double managedLedgerCacheEvictionFrequency = 100.0;
+ @Deprecated
+ private double managedLedgerCacheEvictionFrequency = 100;
+
+ @FieldContext(category = CATEGORY_STORAGE_ML,
+ doc = "Configure the cache eviction interval in milliseconds for
the managed ledger cache."
+ + " This configuration takes effect if bigger than 0,"
+ + " otherwise, managedLedgerCacheEvictionFrequency does")
+ private long managedLedgerCacheEvictionIntervalMs = 0;
Review comment:
Should this take effect by default? So that, we can safely remove
`managedLedgerCacheEvictionFrequency` in future?
##########
File path: conf/broker.conf
##########
@@ -957,8 +957,8 @@ managedLedgerCacheCopyEntries=false
# Threshold to which bring down the cache level when eviction is triggered
managedLedgerCacheEvictionWatermark=0.9
-# Configure the cache eviction frequency for the managed ledger cache
(evictions/sec)
-managedLedgerCacheEvictionFrequency=100.0
+# Configure the cache eviction interval in milliseconds for the managed ledger
cache
+managedLedgerCacheEvictionIntervalMs=0
Review comment:
Should we use this new `managedLedgerCacheEvictionIntervalMs` instead of
`managedLedgerCacheEvictionFrequency` in the default broker.conf?
##########
File path:
pulsar-broker/src/main/java/org/apache/pulsar/broker/ManagedLedgerClientFactory.java
##########
@@ -59,7 +59,11 @@ public void initialize(ServiceConfiguration conf,
MetadataStoreExtended metadata
managedLedgerFactoryConfig.setMaxCacheSize(conf.getManagedLedgerCacheSizeMB() *
1024L * 1024L);
managedLedgerFactoryConfig.setCacheEvictionWatermark(conf.getManagedLedgerCacheEvictionWatermark());
managedLedgerFactoryConfig.setNumManagedLedgerSchedulerThreads(conf.getManagedLedgerNumSchedulerThreads());
-
managedLedgerFactoryConfig.setCacheEvictionFrequency(conf.getManagedLedgerCacheEvictionFrequency());
+ // compatible logic : if managedLedgerCacheEvictionFrequency is
configured,
+ long managedLedgerCacheEvictionIntervalMs =
conf.getManagedLedgerCacheEvictionIntervalMs() > 0
Review comment:
We can override `getManagedLedgerCacheEvictionIntervalMs` method to
handle this compatibility issue like most other config fields does.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
