fabriziofortino commented on code in PR #1215:
URL: https://github.com/apache/jackrabbit-oak/pull/1215#discussion_r1395604781


##########
oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/LucenePropertyIndex.java:
##########
@@ -1644,11 +1647,39 @@ public List<Facet> getFacets(int numberOfFacets, String 
columnName) throws IOExc
                 return cachedResults.get(cacheKey);
             }
             LOG.trace("columnName = {} facet Data not present in cache...", 
columnName);
+            if (EAGER_FACET_CACHE_FILL) {
+                fillFacetCache(numberOfFacets);
+                if (cachedResults.containsKey(cacheKey)) {
+                    LOG.trace("columnName = {} now found");

Review Comment:
   `cacheKey` is missing
   ```suggestion
                       LOG.trace("columnName = {} now found", cacheKey);
   ```



##########
oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/LucenePropertyIndex.java:
##########
@@ -1677,6 +1708,28 @@ private List<Facet> getFacetsUncached(int 
numberOfFacets, String columnName) thr
                 indexNode.release();
             }
         }
+
+        private List<Facet> getFacetsUncached(Facets facets, int 
numberOfFacets, String columnName) throws IOException {
+            String facetFieldName = FulltextIndex.parseFacetField(columnName);
+            try {
+                ImmutableList.Builder<Facet> res = new 
ImmutableList.Builder<>();
+                FacetResult topChildren = 
facets.getTopChildren(numberOfFacets, facetFieldName);
+                if (topChildren == null) {
+                    return null;
+                }
+                for (LabelAndValue lav : topChildren.labelValues) {
+                    res.add(new Facet(
+                            lav.label, lav.value.intValue()
+                    ));
+                }
+                return res.build();
+            } catch (IllegalArgumentException iae) {
+                LOG.debug(iae.getMessage(), iae);
+                LOG.warn("facets for {} not yet indexed: " + iae, 
facetFieldName);

Review Comment:
   this message mixes placeholders + interpolation. I would prefer to use just 
placeholders:
   ```suggestion
                   LOG.warn("facets for {} not yet indexed: {}", 
facetFieldName, iae);
   ```



##########
oak-lucene/src/test/java/org/apache/jackrabbit/oak/plugins/index/lucene/hybrid/ManyFacetsTest.java:
##########
@@ -0,0 +1,280 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.jackrabbit.oak.plugins.index.lucene.hybrid;
+
+import static 
org.apache.jackrabbit.guava.common.util.concurrent.MoreExecutors.newDirectExecutorService;
+import static 
org.apache.jackrabbit.oak.plugins.index.search.FulltextIndexConstants.PROP_FACETS;
+import static 
org.apache.jackrabbit.oak.plugins.index.search.FulltextIndexConstants.STATISTICAL_FACET_SAMPLE_SIZE_DEFAULT;
+import static 
org.apache.jackrabbit.oak.spi.mount.Mounts.defaultMountInfoProvider;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+
+import java.io.File;
+import java.io.IOException;
+import java.util.Properties;
+import java.util.Random;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
+import java.util.function.Predicate;
+
+import javax.jcr.GuestCredentials;
+import javax.jcr.Repository;
+import javax.jcr.RepositoryException;
+import javax.jcr.Session;
+import javax.jcr.query.Query;
+import javax.jcr.query.QueryManager;
+import javax.jcr.query.QueryResult;
+import javax.jcr.query.Row;
+import javax.jcr.query.RowIterator;
+
+import org.apache.jackrabbit.oak.InitialContent;
+import org.apache.jackrabbit.oak.Oak;
+import org.apache.jackrabbit.oak.api.ContentRepository;
+import org.apache.jackrabbit.oak.api.Tree;
+import org.apache.jackrabbit.oak.api.Type;
+import org.apache.jackrabbit.oak.commons.PathUtils;
+import org.apache.jackrabbit.oak.commons.concurrent.ExecutorCloser;
+import org.apache.jackrabbit.oak.commons.json.JsonObject;
+import org.apache.jackrabbit.oak.jcr.Jcr;
+import org.apache.jackrabbit.oak.plugins.index.AsyncIndexUpdate;
+import 
org.apache.jackrabbit.oak.plugins.index.counter.NodeCounterEditorProvider;
+import org.apache.jackrabbit.oak.plugins.index.lucene.IndexCopier;
+import org.apache.jackrabbit.oak.plugins.index.lucene.IndexTracker;
+import 
org.apache.jackrabbit.oak.plugins.index.lucene.LuceneIndexEditorProvider;
+import org.apache.jackrabbit.oak.plugins.index.lucene.LuceneIndexProvider;
+import org.apache.jackrabbit.oak.plugins.index.lucene.LucenePropertyIndex;
+import org.apache.jackrabbit.oak.plugins.index.lucene.TestUtil;
+import 
org.apache.jackrabbit.oak.plugins.index.lucene.TestUtil.OptionalEditorProvider;
+import 
org.apache.jackrabbit.oak.plugins.index.lucene.reader.DefaultIndexReaderFactory;
+import 
org.apache.jackrabbit.oak.plugins.index.lucene.reader.LuceneIndexReaderFactory;
+import 
org.apache.jackrabbit.oak.plugins.index.lucene.util.LuceneIndexDefinitionBuilder;
+import org.apache.jackrabbit.oak.plugins.index.nodetype.NodeTypeIndexProvider;
+import 
org.apache.jackrabbit.oak.plugins.index.property.PropertyIndexEditorProvider;
+import org.apache.jackrabbit.oak.plugins.index.search.FulltextIndexConstants;
+import 
org.apache.jackrabbit.oak.plugins.index.search.util.IndexDefinitionBuilder.PropertyRule;
+import org.apache.jackrabbit.oak.plugins.memory.MemoryNodeStore;
+import org.apache.jackrabbit.oak.query.AbstractQueryTest;
+import org.apache.jackrabbit.oak.spi.commit.Observer;
+import org.apache.jackrabbit.oak.spi.mount.MountInfoProvider;
+import org.apache.jackrabbit.oak.spi.query.QueryIndexProvider;
+import org.apache.jackrabbit.oak.spi.security.OpenSecurityProvider;
+import org.apache.jackrabbit.oak.spi.state.NodeStore;
+import org.apache.jackrabbit.oak.spi.whiteboard.Whiteboard;
+import org.apache.jackrabbit.oak.spi.whiteboard.WhiteboardUtils;
+import org.apache.jackrabbit.oak.stats.Clock;
+import org.apache.jackrabbit.oak.stats.StatisticsProvider;
+import org.jetbrains.annotations.Nullable;
+import org.junit.After;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+
+public class ManyFacetsTest extends AbstractQueryTest {
+
+    @Rule
+    public TemporaryFolder temporaryFolder = new TemporaryFolder(new 
File("target"));
+
+    private static final int NUM_LABELS = 4;
+    private static final int NUM_LEAF_NODES = 
STATISTICAL_FACET_SAMPLE_SIZE_DEFAULT;
+    private static final String FACET_PROP = "facets";
+    private static final long REFRESH_DELTA = TimeUnit.SECONDS.toMillis(1);
+    
+    private static final int FACET_COUNT = 200;
+
+    private ExecutorService executorService = Executors.newFixedThreadPool(2);
+    private OptionalEditorProvider optionalEditorProvider = new 
OptionalEditorProvider();
+    private NRTIndexFactory nrtIndexFactory;
+    private LuceneIndexProvider luceneIndexProvider;
+    private NodeStore nodeStore;
+    private DocumentQueue queue;
+    private Clock clock = new Clock.Virtual();
+    private Whiteboard wb;
+    private QueryManager qm;
+    private Repository jcrRepo;
+    private Jcr jcr;
+    private Oak oak;
+    // backup original system properties i.e. before test started
+    private final Properties backupProperties = (Properties) 
System.getProperties().clone();
+
+    @After
+    public void tearDown() throws IOException {
+        luceneIndexProvider.close();
+        new ExecutorCloser(executorService).close();
+        nrtIndexFactory.close();
+        // restore original system properties i.e. before test started
+        System.setProperties(backupProperties);
+    }
+
+    @Override
+    protected ContentRepository createRepository() {
+        IndexCopier copier;
+        try {
+            copier = new IndexCopier(executorService, 
temporaryFolder.getRoot());
+        } catch (IOException e) {
+            throw new RuntimeException(e);
+        }
+        MountInfoProvider mip = defaultMountInfoProvider();
+        nrtIndexFactory = new NRTIndexFactory(copier, clock, 
TimeUnit.MILLISECONDS.toSeconds(REFRESH_DELTA), StatisticsProvider.NOOP);
+        nrtIndexFactory.setAssertAllResourcesClosed(true);
+        LuceneIndexReaderFactory indexReaderFactory = new 
DefaultIndexReaderFactory(mip, copier);
+        IndexTracker tracker = new IndexTracker(indexReaderFactory, 
nrtIndexFactory);
+        luceneIndexProvider = new LuceneIndexProvider(tracker);
+        queue = new DocumentQueue(100, tracker, newDirectExecutorService());
+        LuceneIndexEditorProvider editorProvider = new 
LuceneIndexEditorProvider(copier,
+                tracker,
+                null,
+                null,
+                mip);
+        editorProvider.setIndexingQueue(queue);
+        LocalIndexObserver localIndexObserver = new LocalIndexObserver(queue, 
StatisticsProvider.NOOP);
+        nodeStore = new MemoryNodeStore();
+        oak = new Oak(nodeStore)
+                .with(new InitialContent())
+                .with(new OpenSecurityProvider())
+                .with((QueryIndexProvider) luceneIndexProvider)
+                .with((Observer) luceneIndexProvider)
+                .with(localIndexObserver)
+                .with(editorProvider)
+                .with(new PropertyIndexEditorProvider())
+                .with(new NodeTypeIndexProvider())
+                .with(optionalEditorProvider)
+                .with(new NodeCounterEditorProvider())
+                //Effectively disable async indexing auto run
+                //such that we can control run timing as per test requirement
+                .withAsyncIndexing("async", TimeUnit.DAYS.toSeconds(1));
+
+        wb = oak.getWhiteboard();
+        ContentRepository repo = oak.createContentRepository();
+        return repo;
+    }
+    
+    private void createSmallDataset(int k) throws RepositoryException {
+        Random random = new Random(42);
+        Tree par = createPath("/parent" + k);
+        par.setProperty("foo", "bar");
+        for (int i = 0; i < NUM_LABELS * 2; i++) {
+            Tree subPar = par.addChild("par" + i);
+            for (int j = 0; j < NUM_LEAF_NODES / (2 * NUM_LABELS); j++) {
+                Tree child = subPar.addChild("c" + j);
+                child.setProperty("cons", "val");
+                for (int f = 0; f < FACET_COUNT; f++) {
+                    int labelNum = random.nextInt(NUM_LABELS);
+                    child.setProperty("foo" + f, "foo"  + f + "x" + labelNum);
+                }
+            }
+        }
+    }
+    
+    private Tree createPath(String path) {
+        Tree base = root.getTree("/");
+        for (String e : PathUtils.elements(path)) {
+            base = base.addChild(e);
+        }
+        return base;
+    }
+    
+    private void runAsyncIndex() {
+        AsyncIndexUpdate async = (AsyncIndexUpdate) 
WhiteboardUtils.getService(wb, Runnable.class, new Predicate<Runnable>() {
+            @Override
+            public boolean test(@Nullable Runnable input) {
+                return input instanceof AsyncIndexUpdate;
+            }
+        });
+        assertNotNull(async);
+        async.run();
+        if (async.isFailing()) {
+            fail("AsyncIndexUpdate failed");
+        }
+        root.refresh();
+    }
+    
+    @Test
+    public void facet() throws Exception {
+        // Explicitly setting following configs to run 
DelayedLuceneFacetProvider and a thread sleep of 50 ms in refresh readers. 
Refer: OAK-8898
+        System.setProperty(LucenePropertyIndex.OLD_FACET_PROVIDER_CONFIG_NAME, 
"false");
+        // The variable is static final so once set it remains same for all 
tests and which will lead to slow execution
+        // of other tests as this add a sleep of specified milliseconds in 
refresh reader method in LuceneIndexNodeManager.
+       // 
System.setProperty(LuceneIndexNodeManager.OLD_FACET_PROVIDER_TEST_FAILURE_SLEEP_INSTRUMENT_NAME,
 "40");

Review Comment:
   nitpick: fix indentation
   ```suggestion
           // 
System.setProperty(LuceneIndexNodeManager.OLD_FACET_PROVIDER_TEST_FAILURE_SLEEP_INSTRUMENT_NAME,
 "40");
   ```



##########
oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/LucenePropertyIndex.java:
##########
@@ -1677,6 +1708,28 @@ private List<Facet> getFacetsUncached(int 
numberOfFacets, String columnName) thr
                 indexNode.release();
             }
         }
+
+        private List<Facet> getFacetsUncached(Facets facets, int 
numberOfFacets, String columnName) throws IOException {

Review Comment:
   `getFacetsUncached(int numberOfFacets, String columnName)` has the exact 
same logic at lines 1690-1705. What about reducing the code in this class (that 
is already huge) and call the new `getFacetsUncached` in there?



##########
oak-lucene/src/test/java/org/apache/jackrabbit/oak/plugins/index/lucene/hybrid/ManyFacetsTest.java:
##########
@@ -0,0 +1,280 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.jackrabbit.oak.plugins.index.lucene.hybrid;
+
+import static 
org.apache.jackrabbit.guava.common.util.concurrent.MoreExecutors.newDirectExecutorService;
+import static 
org.apache.jackrabbit.oak.plugins.index.search.FulltextIndexConstants.PROP_FACETS;
+import static 
org.apache.jackrabbit.oak.plugins.index.search.FulltextIndexConstants.STATISTICAL_FACET_SAMPLE_SIZE_DEFAULT;
+import static 
org.apache.jackrabbit.oak.spi.mount.Mounts.defaultMountInfoProvider;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+
+import java.io.File;
+import java.io.IOException;
+import java.util.Properties;
+import java.util.Random;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
+import java.util.function.Predicate;
+
+import javax.jcr.GuestCredentials;
+import javax.jcr.Repository;
+import javax.jcr.RepositoryException;
+import javax.jcr.Session;
+import javax.jcr.query.Query;
+import javax.jcr.query.QueryManager;
+import javax.jcr.query.QueryResult;
+import javax.jcr.query.Row;
+import javax.jcr.query.RowIterator;
+
+import org.apache.jackrabbit.oak.InitialContent;
+import org.apache.jackrabbit.oak.Oak;
+import org.apache.jackrabbit.oak.api.ContentRepository;
+import org.apache.jackrabbit.oak.api.Tree;
+import org.apache.jackrabbit.oak.api.Type;
+import org.apache.jackrabbit.oak.commons.PathUtils;
+import org.apache.jackrabbit.oak.commons.concurrent.ExecutorCloser;
+import org.apache.jackrabbit.oak.commons.json.JsonObject;
+import org.apache.jackrabbit.oak.jcr.Jcr;
+import org.apache.jackrabbit.oak.plugins.index.AsyncIndexUpdate;
+import 
org.apache.jackrabbit.oak.plugins.index.counter.NodeCounterEditorProvider;
+import org.apache.jackrabbit.oak.plugins.index.lucene.IndexCopier;
+import org.apache.jackrabbit.oak.plugins.index.lucene.IndexTracker;
+import 
org.apache.jackrabbit.oak.plugins.index.lucene.LuceneIndexEditorProvider;
+import org.apache.jackrabbit.oak.plugins.index.lucene.LuceneIndexProvider;
+import org.apache.jackrabbit.oak.plugins.index.lucene.LucenePropertyIndex;
+import org.apache.jackrabbit.oak.plugins.index.lucene.TestUtil;
+import 
org.apache.jackrabbit.oak.plugins.index.lucene.TestUtil.OptionalEditorProvider;
+import 
org.apache.jackrabbit.oak.plugins.index.lucene.reader.DefaultIndexReaderFactory;
+import 
org.apache.jackrabbit.oak.plugins.index.lucene.reader.LuceneIndexReaderFactory;
+import 
org.apache.jackrabbit.oak.plugins.index.lucene.util.LuceneIndexDefinitionBuilder;
+import org.apache.jackrabbit.oak.plugins.index.nodetype.NodeTypeIndexProvider;
+import 
org.apache.jackrabbit.oak.plugins.index.property.PropertyIndexEditorProvider;
+import org.apache.jackrabbit.oak.plugins.index.search.FulltextIndexConstants;
+import 
org.apache.jackrabbit.oak.plugins.index.search.util.IndexDefinitionBuilder.PropertyRule;
+import org.apache.jackrabbit.oak.plugins.memory.MemoryNodeStore;
+import org.apache.jackrabbit.oak.query.AbstractQueryTest;
+import org.apache.jackrabbit.oak.spi.commit.Observer;
+import org.apache.jackrabbit.oak.spi.mount.MountInfoProvider;
+import org.apache.jackrabbit.oak.spi.query.QueryIndexProvider;
+import org.apache.jackrabbit.oak.spi.security.OpenSecurityProvider;
+import org.apache.jackrabbit.oak.spi.state.NodeStore;
+import org.apache.jackrabbit.oak.spi.whiteboard.Whiteboard;
+import org.apache.jackrabbit.oak.spi.whiteboard.WhiteboardUtils;
+import org.apache.jackrabbit.oak.stats.Clock;
+import org.apache.jackrabbit.oak.stats.StatisticsProvider;
+import org.jetbrains.annotations.Nullable;
+import org.junit.After;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+
+public class ManyFacetsTest extends AbstractQueryTest {
+
+    @Rule
+    public TemporaryFolder temporaryFolder = new TemporaryFolder(new 
File("target"));
+
+    private static final int NUM_LABELS = 4;
+    private static final int NUM_LEAF_NODES = 
STATISTICAL_FACET_SAMPLE_SIZE_DEFAULT;
+    private static final String FACET_PROP = "facets";
+    private static final long REFRESH_DELTA = TimeUnit.SECONDS.toMillis(1);
+    
+    private static final int FACET_COUNT = 200;
+
+    private ExecutorService executorService = Executors.newFixedThreadPool(2);
+    private OptionalEditorProvider optionalEditorProvider = new 
OptionalEditorProvider();
+    private NRTIndexFactory nrtIndexFactory;
+    private LuceneIndexProvider luceneIndexProvider;
+    private NodeStore nodeStore;
+    private DocumentQueue queue;
+    private Clock clock = new Clock.Virtual();
+    private Whiteboard wb;
+    private QueryManager qm;
+    private Repository jcrRepo;
+    private Jcr jcr;
+    private Oak oak;
+    // backup original system properties i.e. before test started
+    private final Properties backupProperties = (Properties) 
System.getProperties().clone();
+
+    @After
+    public void tearDown() throws IOException {
+        luceneIndexProvider.close();
+        new ExecutorCloser(executorService).close();
+        nrtIndexFactory.close();
+        // restore original system properties i.e. before test started
+        System.setProperties(backupProperties);
+    }
+
+    @Override
+    protected ContentRepository createRepository() {
+        IndexCopier copier;
+        try {
+            copier = new IndexCopier(executorService, 
temporaryFolder.getRoot());
+        } catch (IOException e) {
+            throw new RuntimeException(e);
+        }
+        MountInfoProvider mip = defaultMountInfoProvider();
+        nrtIndexFactory = new NRTIndexFactory(copier, clock, 
TimeUnit.MILLISECONDS.toSeconds(REFRESH_DELTA), StatisticsProvider.NOOP);
+        nrtIndexFactory.setAssertAllResourcesClosed(true);
+        LuceneIndexReaderFactory indexReaderFactory = new 
DefaultIndexReaderFactory(mip, copier);
+        IndexTracker tracker = new IndexTracker(indexReaderFactory, 
nrtIndexFactory);
+        luceneIndexProvider = new LuceneIndexProvider(tracker);
+        queue = new DocumentQueue(100, tracker, newDirectExecutorService());
+        LuceneIndexEditorProvider editorProvider = new 
LuceneIndexEditorProvider(copier,
+                tracker,
+                null,
+                null,
+                mip);
+        editorProvider.setIndexingQueue(queue);
+        LocalIndexObserver localIndexObserver = new LocalIndexObserver(queue, 
StatisticsProvider.NOOP);
+        nodeStore = new MemoryNodeStore();
+        oak = new Oak(nodeStore)
+                .with(new InitialContent())
+                .with(new OpenSecurityProvider())
+                .with((QueryIndexProvider) luceneIndexProvider)
+                .with((Observer) luceneIndexProvider)
+                .with(localIndexObserver)
+                .with(editorProvider)
+                .with(new PropertyIndexEditorProvider())
+                .with(new NodeTypeIndexProvider())
+                .with(optionalEditorProvider)
+                .with(new NodeCounterEditorProvider())
+                //Effectively disable async indexing auto run
+                //such that we can control run timing as per test requirement
+                .withAsyncIndexing("async", TimeUnit.DAYS.toSeconds(1));
+
+        wb = oak.getWhiteboard();
+        ContentRepository repo = oak.createContentRepository();
+        return repo;
+    }
+    
+    private void createSmallDataset(int k) throws RepositoryException {
+        Random random = new Random(42);
+        Tree par = createPath("/parent" + k);
+        par.setProperty("foo", "bar");
+        for (int i = 0; i < NUM_LABELS * 2; i++) {
+            Tree subPar = par.addChild("par" + i);
+            for (int j = 0; j < NUM_LEAF_NODES / (2 * NUM_LABELS); j++) {
+                Tree child = subPar.addChild("c" + j);
+                child.setProperty("cons", "val");
+                for (int f = 0; f < FACET_COUNT; f++) {
+                    int labelNum = random.nextInt(NUM_LABELS);
+                    child.setProperty("foo" + f, "foo"  + f + "x" + labelNum);
+                }
+            }
+        }
+    }
+    
+    private Tree createPath(String path) {
+        Tree base = root.getTree("/");
+        for (String e : PathUtils.elements(path)) {
+            base = base.addChild(e);
+        }
+        return base;
+    }
+    
+    private void runAsyncIndex() {
+        AsyncIndexUpdate async = (AsyncIndexUpdate) 
WhiteboardUtils.getService(wb, Runnable.class, new Predicate<Runnable>() {
+            @Override
+            public boolean test(@Nullable Runnable input) {
+                return input instanceof AsyncIndexUpdate;
+            }
+        });
+        assertNotNull(async);
+        async.run();
+        if (async.isFailing()) {
+            fail("AsyncIndexUpdate failed");
+        }
+        root.refresh();
+    }
+    
+    @Test
+    public void facet() throws Exception {
+        // Explicitly setting following configs to run 
DelayedLuceneFacetProvider and a thread sleep of 50 ms in refresh readers. 
Refer: OAK-8898
+        System.setProperty(LucenePropertyIndex.OLD_FACET_PROVIDER_CONFIG_NAME, 
"false");
+        // The variable is static final so once set it remains same for all 
tests and which will lead to slow execution
+        // of other tests as this add a sleep of specified milliseconds in 
refresh reader method in LuceneIndexNodeManager.
+       // 
System.setProperty(LuceneIndexNodeManager.OLD_FACET_PROVIDER_TEST_FAILURE_SLEEP_INSTRUMENT_NAME,
 "40");
+        Thread.currentThread().setName("main");
+        String idxName = "hybridtest";
+        Tree idx = createIndex(root.getTree("/"), idxName);
+        TestUtil.enableIndexingMode(idx, 
FulltextIndexConstants.IndexingMode.NRT);
+        setTraversalEnabled(false);
+        root.commit();
+        jcr = new Jcr(oak);
+        jcrRepo = jcr.createRepository();
+        createSmallDataset(0);
+        clock.waitUntil(clock.getTime() + REFRESH_DELTA + 1);
+        root.commit();
+        runAsyncIndex();
+        createSmallDataset(2);
+        clock.waitUntil(clock.getTime() + REFRESH_DELTA + 1);
+        root.commit();
+        Session anonSession = jcrRepo.login(new GuestCredentials());
+        qm = anonSession.getWorkspace().getQueryManager();
+        String facetList = "";
+        for (int i = 0; i < FACET_COUNT; i++) {
+            if (i > 0) {
+                facetList += ", ";
+            }
+            facetList += "[rep:facet(foo" + i + ")]";
+        }
+        String queryString = "SELECT " + facetList + 
+                " FROM [nt:base] WHERE [cons] = 'val'";
+        Query q = qm.createQuery(queryString, SQL2);
+        QueryResult qr = q.execute();
+        try {
+            RowIterator it = qr.getRows();
+            assertTrue(it.hasNext());
+            while (it.hasNext()) {
+                Row r = it.nextRow();
+                for (int i = 0; i < qr.getColumnNames().length; i++) {
+                    String columnName = qr.getColumnNames()[i];
+                    String v = r.getValue(columnName).getString();
+                    JsonObject json = JsonObject.fromJson(v, true);
+                    for (int j = 0; j < NUM_LABELS; j++) {
+                        String n = json.getProperties().get("foo" + i + "x" + 
j);
+                        assertTrue(n != null);

Review Comment:
   `assertNotNull` is a better option here
   ```suggestion
                           assertNotNull(n);
   ```



-- 
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...@jackrabbit.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Reply via email to