OAK-2621: Too many reads for child nodes

git-svn-id: https://svn.apache.org/repos/asf/jackrabbit/oak/trunk@1813171 13f79535-47bb-0310-9956-ffa450edef68

oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeState.java

@@ -268,16 +268,27 @@ public long getChildNodeCount(long max) {
             return bundledChildCount;
         }
 
-        if (max > DocumentNodeStore.NUM_CHILDREN_CACHE_LIMIT) {
-            // count all
-            return Iterables.size(getChildNodeEntries());
+        String name = null;
+        long count = 0;
+        int fetchSize = INITIAL_FETCH_SIZE;
+        long remaining = Math.max(max, 1); // fetch at least once
+        Children c = NO_CHILDREN;
+        while (remaining > 0) {
+            c = store.getChildren(this, name, fetchSize);
+            count += c.children.size();
+            remaining -= c.children.size();
+            if (!c.hasMore) {
+                break;
+            }
+            name = c.children.get(c.children.size() - 1);
+            fetchSize = Math.min(fetchSize << 1, MAX_FETCH_SIZE);
         }
-        Children c = store.getChildren(this, null, (int) max);
-        if (c.hasMore) {
-            return Long.MAX_VALUE;
-        } else {
+        if (!c.hasMore) {
             // we know the exact value
-            return c.children.size() + bundledChildCount;
+            return count + bundledChildCount;
+        } else {
+            // there are more than max
+            return Long.MAX_VALUE;
         }
     }
 

oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStore.java

@@ -147,11 +147,6 @@ public final class DocumentNodeStore
 
     public static final FormatVersion VERSION = FormatVersion.V1_8;
 
-    /**
-     * Do not cache more than this number of children for a document.
-     */
-    static final int NUM_CHILDREN_CACHE_LIMIT = Integer.getInteger("oak.documentMK.childrenCacheLimit", 16 * 1024);
-
     /**
      * List of meta properties which are created by DocumentNodeStore and which needs to be
      * retained in any cloned copy of DocumentNodeState.

oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStoreTest.java

@@ -16,6 +16,7 @@
  */
 package org.apache.jackrabbit.oak.plugins.document;
 
+import static com.google.common.collect.ImmutableList.of;
 import static java.util.Collections.emptyList;
 import static java.util.Collections.synchronizedList;
 import static java.util.concurrent.TimeUnit.SECONDS;
@@ -52,6 +53,7 @@
 import java.util.Date;
 import java.util.HashMap;
 import java.util.HashSet;
+import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
 import java.util.Random;
@@ -3549,7 +3551,6 @@ public void diffWithBrokenJournal() throws Exception {
     }
 
     // OAK-2621
-    @Ignore("OAK-2621")
     @Test
     public void getChildNodeCount() throws Exception {
         CountingDocumentStore store = new CountingDocumentStore(new MemoryDocumentStore());
@@ -3573,6 +3574,56 @@ public void getChildNodeCount() throws Exception {
         assertEquals(1, store.getNumQueryCalls(NODES));
     }
 
+    @Test
+    public void getChildNodeCountTest() throws Exception {
+        final long UL = Long.MAX_VALUE; // unknown
+        // childNodeCount = none
+        getChildNodeCountTest(0,
+                of(0L, 1L),
+                of(0L, 0L)
+        );
+        // childNodeCount = less than initial fetch size 42
+        getChildNodeCountTest(42,
+                of( 0L,  1L, 41L, 42L, 43L, 100L),
+                of(42L, 42L, 42L, 42L, 42L, 42L)
+        );
+        // childNodeCount = initial fetch size (100)
+        getChildNodeCountTest(100,
+                of(  0L,   1L,  99L, 100L, 101L, 200L),
+                of(100L, 100L, 100L, 100L, 100L, 100L)
+        );
+        // childNodeCount = initial fetch size + 1 (100 + 1)
+        getChildNodeCountTest(101,
+                of(0L, 1L, 99L, 100L, 101L, 200L),
+                of(UL, UL,  UL,   UL, 101L, 101L)
+        );
+        // childNodeCount = first two fetches (100 + 200)
+        getChildNodeCountTest(300,
+                of(0L, 1L, 99L, 100L, 101L, 200L, 299L, 300L, 301L, 400L),
+                of(UL, UL,  UL,   UL, 300L, 300L, 300L, 300L, 300L, 300L)
+        );
+    }
+
+    private void getChildNodeCountTest(int numChildren,
+                                       Iterable<Long> maxValues,
+                                       Iterable<Long> expectedValues)
+            throws Exception {
+        DocumentNodeStore ns = builderProvider.newBuilder()
+                .setAsyncDelay(0).getNodeStore();
+        NodeBuilder builder = ns.getRoot().builder();
+        for (int i = 0; i < numChildren; i++) {
+            builder.child("test").child("node-" + i);
+        }
+        merge(ns, builder);
+        ns.getNodeChildrenCache().invalidateAll();
+
+        NodeState test = ns.getRoot().getChildNode("test");
+        Iterator<Long> expected = expectedValues.iterator();
+        for (long max : maxValues) {
+            assertEquals(expected.next().longValue(), test.getChildNodeCount(max));
+        }
+    }
+
     private static class WriteCountingStore extends MemoryDocumentStore {
         private final ThreadLocal<Boolean> createMulti = new ThreadLocal<>();
         int count;

oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/GetChildNodeCountTest.java

@@ -0,0 +1,93 @@
+/*
+ * 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.document;
+
+import javax.jcr.SimpleCredentials;
+
+import org.apache.jackrabbit.oak.Oak;
+import org.apache.jackrabbit.oak.api.ContentRepository;
+import org.apache.jackrabbit.oak.api.ContentSession;
+import org.apache.jackrabbit.oak.api.Root;
+import org.apache.jackrabbit.oak.api.Tree;
+import org.apache.jackrabbit.oak.api.Type;
+import org.apache.jackrabbit.oak.plugins.document.memory.MemoryDocumentStore;
+import org.apache.jackrabbit.oak.plugins.index.property.PropertyIndexEditorProvider;
+import org.apache.jackrabbit.oak.spi.security.OpenSecurityProvider;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+
+import static com.google.common.collect.Lists.newArrayList;
+import static org.apache.jackrabbit.oak.plugins.document.Collection.NODES;
+import static org.apache.jackrabbit.oak.plugins.document.util.Utils.closeIfCloseable;
+import static org.hamcrest.Matchers.lessThan;
+import static org.junit.Assert.assertThat;
+
+public class GetChildNodeCountTest {
+
+    @Rule
+    public DocumentMKBuilderProvider builderProvider = new DocumentMKBuilderProvider();
+
+    private CountingDocumentStore store = new CountingDocumentStore(new MemoryDocumentStore());
+
+    private DocumentNodeStore ns;
+
+    private ContentRepository repository;
+
+    private ContentSession session;
+
+    @Before
+    public void before() throws Exception {
+        ns = builderProvider.newBuilder().setAsyncDelay(0)
+                .setDocumentStore(store).getNodeStore();
+        repository = new Oak(ns)
+                .with(new PropertyIndexEditorProvider())
+                .with(new OpenSecurityProvider())
+                .createContentRepository();
+        session = repository.login(new SimpleCredentials("admin", "admin".toCharArray()), null);
+        Root root = session.getLatestRoot();
+        Tree idx = root.getTree("/").addChild("oak:index").addChild("p");
+        idx.setProperty("type", "property");
+        idx.setProperty("propertyNames", newArrayList("p"), Type.NAMES);
+        root.commit();
+    }
+
+    @After
+    public void after() throws Exception {
+        session.close();
+        closeIfCloseable(repository);
+        ns.dispose();
+    }
+
+    @Test
+    public void removeIndexedNodes() throws Exception {
+        Root root = session.getLatestRoot();
+        Tree t = root.getTree("/").addChild("test");
+        for (int i = 0; i < 200; i ++) {
+            t.addChild("node-" + i).setProperty("p", "v");
+        }
+        root.commit();
+        ns.getNodeChildrenCache().invalidateAll();
+        store.resetCounters();
+        // remove nodes
+        root = session.getLatestRoot();
+        root.getTree("/test").remove();
+        root.commit();
+        assertThat(store.getNumQueryCalls(NODES), lessThan(13));
+    }
+}