[GitHub] [pulsar] eolivelli commented on a change in pull request #8474: Support taking de-duplication snapshots based on time

[GitBox <gi...@apache.org>]

eolivelli commented on a change in pull request #8474:
URL: https://github.com/apache/pulsar/pull/8474#discussion_r519353285



##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java
##########
@@ -297,6 +298,8 @@ public BrokerService(PulsarService pulsar) throws Exception {
                 .newSingleThreadScheduledExecutor(new DefaultThreadFactory("consumed-Ledgers-monitor"));
         this.ledgerFullMonitor =
                 Executors.newSingleThreadScheduledExecutor(new DefaultThreadFactory("ledger-full-monitor"));
+        this.deduplicationSnapshotMonitor =

Review comment:
       Can we save creating the pool if the feature is disabled

##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java
##########
@@ -297,6 +298,8 @@ public BrokerService(PulsarService pulsar) throws Exception {
                 .newSingleThreadScheduledExecutor(new DefaultThreadFactory("consumed-Ledgers-monitor"));
         this.ledgerFullMonitor =
                 Executors.newSingleThreadScheduledExecutor(new DefaultThreadFactory("ledger-full-monitor"));
+        this.deduplicationSnapshotMonitor =

Review comment:
       It looks like we are not shutting down the pool, leaking threads

##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java
##########
@@ -413,6 +413,18 @@
     )
     private int brokerDeduplicationMaxNumberOfProducers = 10000;
 
+    @FieldContext(
+        category = CATEGORY_POLICIES,
+        doc = "How often is the thread pool scheduled to check whether a snapshot needs to be taken.(disable with value 0)"
+    )
+    private int brokerDeduplicationSnapshotFrequencyInSeconds = 120;
+    @FieldContext(
+        category = CATEGORY_POLICIES,
+        doc = "If this time interval is exceeded, a snapshot will be taken."
+            + "It will run simultaneously with `brokerDeduplicationEntriesInterval`"
+    )
+    private Integer brokerDeduplicationSnapshotIntervalSeconds = 120;

Review comment:
       Why Integer and not int?




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org