joerghoh commented on code in PR #125:
URL:
https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/125#discussion_r1819048628
##########
src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java:
##########
@@ -242,7 +260,16 @@ 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);
+ }
+ if (invalidAliases.size() >= MAX_REPORT_DEFUNCT_ALIASES) {
+ log.warn("There are {} invalid aliases; excerpt: {}",
invalidAliases.size(), invalidAliases);
Review Comment:
```suggestion
log.warn("There are more than {} invalid aliases;
excerpt: {}", invalidAliases.size(), invalidAliases);
```
##########
src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverMetrics.java:
##########
@@ -85,18 +87,27 @@ public class ResourceResolverMetrics {
private ServiceRegistration<Gauge<Long>>
numberOfResourcesWithAliasesOnStartupGauge;
private Supplier<Long> numberOfResourcesWithAliasesOnStartupSupplier =
ZERO_SUPPLIER;
- private Counter unclosedResourceResolvers;
+ // total number of detected invalid aliases on startup
+ private ServiceRegistration<Gauge<Long>>
numberOfDetectedInvalidAliasesGauge;
+ private Supplier<Long> numberOfDetectedInvalidAliasesSupplier =
ZERO_SUPPLIER;
+
+ // total number of detected conflicting aliases on startup
+ private ServiceRegistration<Gauge<Long>>
numberOfDetectedConflictingAliasesGauge;
+ private Supplier<Long> numberOfDetectedConflictingAliasesSupplier =
ZERO_SUPPLIER;
+ private Counter unclosedResourceResolvers;
@Activate
protected void activate(BundleContext bundleContext) {
- numberOfVanityPathsGauge = registerGauge(bundleContext, METRICS_PREFIX
+ ".numberOfVanityPaths", () -> numberOfVanityPathsSupplier );
- numberOfResourcesWithVanityPathsOnStartupGauge =
registerGauge(bundleContext, METRICS_PREFIX +
".numberOfResourcesWithVanityPathsOnStartup", () ->
numberOfResourcesWithVanityPathsOnStartupSupplier );
- numberOfVanityPathLookupsGauge = registerGauge(bundleContext,
METRICS_PREFIX + ".numberOfVanityPathLookups", () ->
numberOfVanityPathLookupsSupplier );
- numberOfVanityPathBloomNegativeGauge = registerGauge(bundleContext,
METRICS_PREFIX + ".numberOfVanityPathBloomNegatives", () ->
numberOfVanityPathBloomNegativeSupplier );
- numberOfVanityPathBloomFalsePositiveGauge =
registerGauge(bundleContext, METRICS_PREFIX +
".numberOfVanityPathBloomFalsePositives", () ->
numberOfVanityPathBloomFalsePositiveSupplier );
+ numberOfVanityPathsGauge = registerGauge(bundleContext, METRICS_PREFIX
+ ".numberOfVanityPaths", () -> numberOfVanityPathsSupplier);
+ numberOfResourcesWithVanityPathsOnStartupGauge =
registerGauge(bundleContext, METRICS_PREFIX +
".numberOfResourcesWithVanityPathsOnStartup", () ->
numberOfResourcesWithVanityPathsOnStartupSupplier);
+ numberOfVanityPathLookupsGauge = registerGauge(bundleContext,
METRICS_PREFIX + ".numberOfVanityPathLookups", () ->
numberOfVanityPathLookupsSupplier);
+ numberOfVanityPathBloomNegativeGauge = registerGauge(bundleContext,
METRICS_PREFIX + ".numberOfVanityPathBloomNegative", () ->
numberOfVanityPathBloomNegativeSupplier);
+ numberOfVanityPathBloomFalsePositiveGauge =
registerGauge(bundleContext, METRICS_PREFIX +
".numberOfVanityPathBloomFalsePositive", () ->
numberOfVanityPathBloomFalsePositiveSupplier);
Review Comment:
did you just remove the trailing ``s`` character from the name of the
already metric? While technically an API breaking change, this could break
anyone already using this metric.
##########
src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java:
##########
@@ -242,7 +260,16 @@ 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:
```suggestion
log.warn("There are more than {} conflicting
aliases; excerpt: {}", conflictingAliases.size(), conflictingAliases);
```
##########
src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java:
##########
@@ -242,7 +260,16 @@ 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) {
Review Comment:
I see it right below, elements are only added to conflictingAliases while
``conflicting < MAX_REPORT_DEFUNCT_ALIASES``. that means
``conflictingAliases.size()`` is always less than
``MAX_REPORT_DEFUNCT_ALIASES``.
I would always log conflicts, even if we have only 5.
##########
src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java:
##########
@@ -1266,17 +1293,27 @@ private boolean loadAlias(final Resource resource,
Map<String, Map<String, Colle
// the order matters here, the first alias in the array must
come first
for (final String alias : aliasArray) {
if (isAliasInvalid(alias)) {
- log.warn("Encountered invalid alias '{}' under parent
path '{}'. Refusing to use it.", alias, parentPath);
+ long invalids =
detectedInvalidAliases.incrementAndGet();
+ log.warn("Encountered invalid alias '{}' under parent
path '{}' (total so far: {}). Refusing to use it.",
+ alias, parentPath, invalids);
+ if (invalids < MAX_REPORT_DEFUNCT_ALIASES) {
+ invalidAliases.add((String.format("'%s'/'%s'",
parentPath, alias)));
+ }
} else {
Map<String, Collection<String>> parentMap =
map.computeIfAbsent(parentPath, key -> new ConcurrentHashMap<>());
Optional<String> siblingResourceNameWithDuplicateAlias
= parentMap.entrySet().stream()
.filter(entry ->
!entry.getKey().equals(resourceName)) // ignore entry for the current resource
.filter(entry ->
entry.getValue().contains(alias))
.findFirst().map(Map.Entry::getKey);
if (siblingResourceNameWithDuplicateAlias.isPresent())
{
+ long conflicting =
detectedConflictingAliases.incrementAndGet();
log.warn(
- "Encountered duplicate alias '{}' under
parent path '{}'. Refusing to replace current target {} with {}.",
- alias, parentPath,
siblingResourceNameWithDuplicateAlias.get(), resourceName);
+ "Encountered duplicate alias '{}' under
parent path '{}'. Refusing to replace current target '{}' with '{}' (total so
far: {}).",
Review Comment:
```suggestion
"Encountered duplicate alias '{}' under
parent path '{}'. Refusing to replace current target '{}' with '{}' (total
duplicated aliases so far: {}).",
```
##########
src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java:
##########
@@ -242,7 +260,16 @@ 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);
+ }
+ if (invalidAliases.size() >= MAX_REPORT_DEFUNCT_ALIASES) {
Review Comment:
same as above.
##########
src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java:
##########
@@ -242,7 +260,16 @@ 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);
+ }
+ if (invalidAliases.size() >= MAX_REPORT_DEFUNCT_ALIASES) {
+ log.warn("There are {} invalid aliases; excerpt: {}",
invalidAliases.size(), invalidAliases);
+ }
+ conflictingAliases.clear();
Review Comment:
why do you clear these structures? From what I understood, this code is
executed only once for the lifetime of the ResourceResolverFactory, so it's not
necessary at all. Nothing against it (easy saved bytes), but a comment would
explain why this is done here.
--
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]
