jsedding commented on code in PR #107:
URL:
https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/107#discussion_r1401862039
##########
src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java:
##########
@@ -140,7 +142,10 @@ public class MapEntries implements
private Map <String,List <String>> vanityTargets;
- private Map<String, Map<String, String>> aliasMap;
+ /**
+ * The key of the map is the parent path, while the value is a map with
the the resource name as key and the actual aliases as values)
+ */
+ private ConcurrentMap<String, ConcurrentMap<String, List<String>>>
aliasMapsMap;
Review Comment:
If I understand correctly you're changing the data structure from
"Map<parentPath, Map<aliasedName, resourceName>" to "Map<parentPath,
Map<resourceName, List<aliasedName>>". Is that correct? If that is the case, I
like the more compact storage. Just be aware, however, that it makes "lookup by
alias" slightly more expensive, as it requires traversal of the entire map
returned for a parentPath, including a `Collection#contains(String)` call on
the `List`. On the flip side, it is cheaper to generate all aliased paths.
Rgd. the implementation, I would avoid using `ConcurrentMap` in the type
definition (unless we use some of its API), and rather use `Map<String,
Map<String, List<String>>`.
I am also wondering about the nature of the `List<String>`, i.e. the aliases
IIUC.
- Do we need to preserve their order?
- Do we need to maintain duplicates?
- Is the list updated or replaced? And, given the above questions, would it
make sense to replace rather than update it?
Depending on the answers, we could consider using a `ConcurrentSkipListSet`,
`Collections.newSetFromMap(new ConcurrentHashMap<>())` or an immutable `Set`,
e.g. via `Set.of()`. In any case, an implementation that has a fast
`Collection#contains(String)` implementation would be good.
##########
src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java:
##########
@@ -18,32 +18,6 @@
*/
package org.apache.sling.resourceresolver.impl.mapping;
-import org.apache.commons.collections4.map.LRUMap;
Review Comment:
I would prefer limiting reordering of the imports.
--
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]
