kwin commented on code in PR #107:
URL:
https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/107#discussion_r1402245165
##########
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>". 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.
Yes, this is all correct. I consider the case to lookup an alias for a given
resource name (i.e. map) more frequent than looking for an alias below a
specific parent (i.e. resolve). The flipping of keys and values is necessary to
keep the order of the `sling:alias` values for a specific resource (see next
question) as there is no ConcurrentMap keeping the insertion order (similar to
LinkedHashMap).
> 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>>.
This was done deliberately to make users aware that this is a thread safe
implementation, i.e. no need to synchronize access on it. This is more explicit
than just mentioning it in the javadoc. What issues do you see we that?
ConcurrentMap and Map share exactly the same interface signature, the
differences are only in the default implementations.
> Do we need to preserve their order?
Yes, because the first entry is used for Outgoing Mapping
> Do we need to maintain duplicates?
Only to emit a warn once if there is a duplicate found in siblings. The
duplicates are just discarded, this is exactly the behaviour as before
> Is the list updated or replaced? And, given the above questions, would it
make sense to replace rather than update it?
The implementation used here `CopyOnWriteArrayList` uses a replacement
internally. This is easier to deal with than recreating the list manually for
updates. For me this impl is reasonable here, because we can only expect at
most a handful of items per list and this impl. doesn't come with any
performance drawback during reading, just a small one when updating (due to the
copy of the list)
--
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: dev-unsubscr...@sling.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
