rombert commented on code in PR #125:
URL:
https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/125#discussion_r1819299913
##########
src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverMetrics.java:
##########
@@ -36,14 +36,16 @@
* Export metrics for the resource resolver bundle:
*
* org.apache.sling.resourceresolver.numberOfResourcesWithAliasesOnStartup --
the total number of resources with sling:alias properties found on startup
+ * org.apache.sling.resourceresolver.numberOfDetectedConflictingAliases --
the total number of detected invalid aliases
+ * org.apache.sling.resourceresolver.numberOfDetectedInvalidAliases -- the
total number of detected invalid aliases
+ *
*
org.apache.sling.resourceresolver.numberOfResourcesWithVanityPathsOnStartup --
the total number of resources with sling:vanityPath properties found on startup
- * org.apache.sling.resourceresolver.numberOfVanityPaths -- the total number
of vanity paths in the cache
Review Comment:
This metric name is now missing from the comment, is that intended?
##########
src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java:
##########
@@ -242,7 +260,21 @@ protected boolean doInit() {
try {
final Map<String, Map<String, Collection<String>>>
loadedMap = this.loadAliases(resolver);
this.aliasMapsMap = loadedMap;
-
+
+ // warn if there are more than a few defunct aliases
+ if (conflictingAliases.size() >=
MAX_REPORT_DEFUNCT_ALIASES) {
+ log.warn("There are {} {} conflicting aliases;
excerpt: {}", conflictingAliases.size(), conflictingAliases);
Review Comment:
Looks like there are three placeholders but only two values.
##########
src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java:
##########
@@ -179,7 +188,12 @@ public MapEntries(final MapConfigurationProvider factory,
this.aliasMapsMap = new ConcurrentHashMap<>();
this.stringInterpolationProvider = stringInterpolationProvider;
+ this.conflictingAliases = new ConcurrentLinkedQueue<>();
+ this.invalidAliases = new ConcurrentLinkedQueue<>();
Review Comment:
Do `this.conflictingAliases` and `this.invalidAliases` really need to be
fields and concurret collections? An approach where they would be used on a
single thread and within a single method (or passed around methods) would IMO
make it much easier to reason about the concurrency model of this class.
--
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]
