[jira] [Commented] (OAK-3826) Lucene index augmentation doesn't work in Osgi environment

["Chetan Mehrotra (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/OAK-3826?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15094074#comment-15094074 ] 

Chetan Mehrotra commented on OAK-3826:
--------------------------------------

Approach looks fine. Some minor comments
# Omit metatype - As for now this component does not have any config to be edited
{code}
+@Component(metatype = true, label = "Apache Jackrabbit Oak IndexAugmentorFactory")
{code}
# You can omit bind/unbind as the _The default value is the name created by appending the reference name to the string unbind_
{code}
+        @Reference(name = "IndexFieldProvider",
+                policy = ReferencePolicy.DYNAMIC,
+                cardinality = ReferenceCardinality.OPTIONAL_MULTIPLE,
+                referenceInterface = IndexFieldProvider.class,
+                bind = "bindIndexFieldProviderService",
+                unbind = "unbindIndexFieldProviderService")
{code}
# Visibility of state in {{IndexAugmentorFactory}} - Keep them provide. And provide a method to enable assert if they are all reset to empty
# Use {{com.google.common.collect.Sets#newIdentityHashSet}} which would be safer and not depend on equals of service impls
{code}
+    public IndexAugmentorFactory() {
+        indexFieldProviderSet = Sets.newHashSet();
+        fulltextQueryTermsProviderSet = Sets.newHashSet();
{code}
# Instead of directly assigning {{tempMap}} to {{indexFieldProviderMap}} use {{com.google.common.collect.ImmutableMap#copyOf}}. This provides immutable variant and fast access for single entry setup!
{code}
 Map<String, CompositeIndexFieldProvider> tempMap = Maps.newHashMap();
        for (String nodeType : indexFieldProviderListMultimap.keySet()) {
            List<IndexFieldProvider> providers = indexFieldProviderListMultimap.get(nodeType);
            CompositeIndexFieldProvider compositeIndexFieldProvider =
                    new CompositeIndexFieldProvider(nodeType, providers);
            tempMap.put(nodeType, compositeIndexFieldProvider);
        }

        indexFieldProviderMap = tempMap;
{code}
# CompositeIndexFieldProvider - It might be safer to make use of ImmutableList to copy the list. Not sure the semantics of list from a listMultiMap (may be a view?)

> Lucene index augmentation doesn't work in Osgi environment
> ----------------------------------------------------------
>
>                 Key: OAK-3826
>                 URL: https://issues.apache.org/jira/browse/OAK-3826
>             Project: Jackrabbit Oak
>          Issue Type: Bug
>          Components: lucene
>            Reporter: Vikas Saurabh
>            Assignee: Vikas Saurabh
>            Priority: Minor
>             Fix For: 1.4
>
>         Attachments: OAK-3826-v2.patch, OAK-3826.patch
>
>
> OAK-3576 introduced a way to hook SPI to provide extra fields and query terms for a lucene index.
> In Osgi world, due to OAK-3815, {{LuceneIndexProviderService}} registered references to SPI and pinged {{IndexAugmentFactory}} to update its map. But, it seems bind/unbind methods get called ahead of time as compared to the information Tracker contains. This leads to wrong set of services captured by {{IndexAugmentFactory}}



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)