[GitHub] [sling-org-apache-sling-distribution-journal] cschneider commented on a change in pull request #63: SLING-10107 [Refactoring] improve resourceresolver handling

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

cschneider commented on a change in pull request #63:
URL: https://github.com/apache/sling-org-apache-sling-distribution-journal/pull/63#discussion_r811964442



##########
File path: src/main/java/org/apache/sling/distribution/journal/bookkeeper/BookKeeper.java
##########
@@ -113,16 +117,33 @@ public BookKeeper(
         this.config = config;
         String nameRetries = DistributionMetricsService.SUB_COMPONENT + ".current_retries;sub_name=" + config.getSubAgentName();
         this.retriesGauge = distributionMetricsService.createGauge(nameRetries, "Retries of current package", packageRetries::getSum);
+
+        this.resolversOpenedGauge = distributionMetricsService
+                .createGauge(DistributionMetricsService.SUB_COMPONENT + ".openedResolvers;sub_name=" + config.getSubAgentName(),
+                 "number of opened ResourceResolvers for this bookkeeper", resolverOpenCounter::get);
         this.resolverFactory = resolverFactory;
         this.distributionMetricsService = distributionMetricsService;
         // Error queues are enabled when the number
         // of retry attempts is limited ; disabled otherwise
         this.errorQueueEnabled = (config.getMaxRetries() >= 0);
-        this.statusStore = new LocalStore(resolverFactory, STORE_TYPE_STATUS, config.getSubAgentName());
-        this.processedOffsets = new LocalStore(resolverFactory, STORE_TYPE_PACKAGE, config.getSubAgentName());
+        this.statusStore = new LocalStore(bookKeeperResolver, STORE_TYPE_STATUS, config.getSubAgentName());
+        this.processedOffsets = new LocalStore(bookKeeperResolver, STORE_TYPE_PACKAGE, config.getSubAgentName());
         log.info("Started bookkeeper {}.", config);
     }
-    
+
+    /**
+     * Centralized way to obtain a resourceResolver for the subservice "bookkeeper"
+     */
+    protected Supplier<ResourceResolver> bookKeeperResolver = () -> {
+        try {
+            resolverOpenCounter.incrementAndGet();
+            return resolverFactory.getServiceResourceResolver(singletonMap(SUBSERVICE, SUBSERVICE_BOOKKEEPER));
+        } catch (LoginException e) {
+            log.error("Cannot open ResourceResolver", e);
+            return null;

Review comment:
       I think returning null is dangerous here as it later leads to a NPE. How about throwing an exception instead?

##########
File path: src/main/java/org/apache/sling/distribution/journal/bookkeeper/LocalStore.java
##########
@@ -86,18 +83,18 @@ public synchronized void store(ResourceResolver serviceResolver, Map<String, Obj
         LOG.debug(String.format("Stored data %s for storeId %s", map.toString(), storeId));
     }
 
-    public <T> T load(String key, Class<T> clazz) {
-        return load().get(key, clazz);
+    public <T> T load(Supplier<ResourceResolver> supplier, String key, Class<T> clazz) {
+        return load(supplier).get(key, clazz);
     }
 
-    public <T> T load(String key, T defaultValue) {
+    public <T> T load(Supplier<ResourceResolver> supplier, String key, T defaultValue) {
         LOG.debug(String.format("Loading key %s for storeId %s with default value %s", key, storeId, defaultValue));
-        return load().get(key, defaultValue);
+        return load(supplier).get(key, defaultValue);
     }
 
-    public ValueMap load() {
+    public ValueMap load(Supplier<ResourceResolver> supplier) {

Review comment:
       When using a LocalStore I think it is a bad idea to provide a ResourceResolver supplier. Why do you think this is necessary?




-- 
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: commits-unsubscribe@sling.apache.org

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