SLING-10107 [Refactoring] improve resourceresolver handling #63
Conversation
Use a Supplier so the LocalStore does not need to know about the creation of the ResourceResolver and its parameters.
Whenever a resourceResolver for the bookkeeper subservice is required, a single central supplier is used. This supplier also exposes a gauge metric so it can be observed how often it is invoked.
|
Kudos, SonarCloud Quality Gate passed! |
There was a problem hiding this comment.
I like the idea of externalising the ResourceResolver creation with a Supplier. We should not leak this into the load methods though.
As a further improvement I think we could have a factory class for the resource resolver outside of BookKeeper. The factory could be a DS component that gets the ResourceResolverFactory injected and could provide a supplier via the create method.
| return resolverFactory.getServiceResourceResolver(singletonMap(SUBSERVICE, SUBSERVICE_BOOKKEEPER)); | ||
| } catch (LoginException e) { | ||
| log.error("Cannot open ResourceResolver", e); | ||
| return null; |
There was a problem hiding this comment.
I think returning null is dangerous here as it later leads to a NPE. How about throwing an exception instead?
| } | ||
|
|
||
| public ValueMap load() { | ||
| public ValueMap load(Supplier<ResourceResolver> supplier) { |
There was a problem hiding this 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.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()); |
There was a problem hiding this comment.
What's the lifetime of a bookkeeper object? If it's a long-living object, these ResourceResolvers are also long-living, which is a anti-pattern in Sling/JCR.
There was a problem hiding this comment.
BookKeeper is long lived. We are only passing resolverFactory there though. The ResourceResolvers should be short lived.
There was a problem hiding this comment.
BookKeeper is long lived, but the resources resolvers it builds inside the LocalStore are short lived and kept private. I think the current implementation is good and simple.
|
I like the idea of having a metric of resourceResolvers. How about putting the metric inside the standard ResourceResolverFactory in sling? |
|
@cschneider you mean a metric into the ResourceResolverFactory? For Oak-based setups we have a metric already there. |
|
Would this metric already help to diagnose if we open too many resource resolvers? |
I refactored the creation of ResourceResolvers (for the bookkeeper subservice) and moved it into a central supplier, which allows also a better monitoring how often ResourceResolvers are actually opened here.
If necessary, further optimizations can be done from here regarding a more efficient use of ResourceResolvers.