thomasmueller commented on code in PR #914:
URL: https://github.com/apache/jackrabbit-oak/pull/914#discussion_r1184915356
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/counter/NodeCounterEditor.java:
##########
@@ -101,6 +118,19 @@ private SipHash getHash() {
return h;
}
+ /*
+ If an instance is restarted, the nodeCounterMetric might be 0 when we
retrieve it as its value isn't persisted.
Review Comment:
It makes sense to use a Javadoc comment here, using /**
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/counter/NodeCounterEditorProvider.java:
##########
@@ -48,6 +49,14 @@ public class NodeCounterEditorProvider implements
IndexEditorProvider {
@Reference
private MountInfoProvider mountInfoProvider =
Mounts.defaultMountInfoProvider();
+ /*
+ the statistics provider will be null for any testcase that is using the
nodeCounter. As some of these testcases are
Review Comment:
/** and then uppercase T
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/counter/NodeCounterEditor.java:
##########
@@ -85,6 +97,11 @@ private NodeCounterEditor(NodeCounterRoot root,
NodeCounterEditor parent, String
this.currentMount = this.parent.currentMount;
this.mountCanChange = false;
}
+
+ this.statisticsProvider = statisticsProvider;
+ this.nodeCounterMetric =
initNodeCounterMetric(statisticsProvider.getCounterStats(NODE_COUNT_FROM_ROOT,
StatsOptions.DEFAULT),
+ root.root,
Review Comment:
I would add a newline in this case, if the line is very long, after the (
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/counter/NodeCounterEditor.java:
##########
@@ -41,15 +44,20 @@
public class NodeCounterEditor implements Editor {
public static final String DATA_NODE_NAME = ":index";
-
+
Review Comment:
It is not wrong, but generally I would avoid doing changes that only affect
whitespace. To make reviews faster.
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/counter/NodeCounterEditor.java:
##########
@@ -177,6 +207,8 @@ private void leaveNew() throws CommitFailedException {
}
} else {
builder.setProperty(COUNT_HASH_PROPERTY_NAME, count);
+ long delta = count - nodeCounterMetric.getCount();
Review Comment:
I think this will update the metric for each node, but you only want to do
that for the root (top level)
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/counter/NodeCounterEditorProvider.java:
##########
@@ -83,12 +92,22 @@ public Editor getIndexEditor(@NotNull String type,
} else {
NodeCounterEditor.NodeCounterRoot rootData = new
NodeCounterEditor.NodeCounterRoot(
resolution, seed, definition, root, callback);
- return new NodeCounterEditor(rootData, mountInfoProvider);
+ return new NodeCounterEditor(rootData, mountInfoProvider,
statisticsProvider);
}
}
public NodeCounterEditorProvider with(MountInfoProvider mountInfoProvider)
{
this.mountInfoProvider = mountInfoProvider;
return this;
}
+
+ /*
+ This is mostly intended to be used during testing if there is a need to
use a statistics provider that will be
Review Comment:
/**
##########
oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/counter/NodeCounterMetricTest.java:
##########
@@ -0,0 +1,191 @@
+/*
+ * 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.counter;
+
+import org.apache.jackrabbit.oak.InitialContent;
+import org.apache.jackrabbit.oak.Oak;
+import org.apache.jackrabbit.oak.api.Tree;
+import org.apache.jackrabbit.oak.api.CommitFailedException;
+import org.apache.jackrabbit.oak.api.Root;
+import org.apache.jackrabbit.oak.api.ContentSession;
+import org.apache.jackrabbit.oak.commons.concurrent.ExecutorCloser;
+import org.apache.jackrabbit.oak.plugins.index.AsyncIndexUpdate;
+import org.apache.jackrabbit.oak.plugins.index.counter.jmx.NodeCounter;
+import
org.apache.jackrabbit.oak.plugins.index.property.PropertyIndexEditorProvider;
+import org.apache.jackrabbit.oak.plugins.memory.MemoryNodeStore;
+import org.apache.jackrabbit.oak.plugins.metric.MetricStatisticsProvider;
+import org.apache.jackrabbit.oak.spi.security.OpenSecurityProvider;
+import org.apache.jackrabbit.oak.spi.state.NodeStateUtils;
+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.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+
+import javax.jcr.NoSuchWorkspaceException;
+import javax.management.ObjectName;
+import javax.management.MalformedObjectNameException;
+import javax.management.MBeanServerConnection;
+import javax.management.ReflectionException;
+import javax.management.InstanceNotFoundException;
+import javax.management.AttributeNotFoundException;
+import javax.management.MBeanException;
+import javax.security.auth.login.LoginException;
+import java.io.IOException;
+import java.lang.management.ManagementFactory;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.TimeUnit;
+import java.util.function.Predicate;
+
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.assertEquals;
+
+
+public class NodeCounterMetricTest {
+ private ScheduledExecutorService executor;
+ private MetricStatisticsProvider statsProvider;
+ private static final String mBeanName =
"org.apache.jackrabbit.oak:name=NODE_COUNT_FROM_ROOT,type=Metrics";
+ private ObjectName mBeanObjectName;
+
+ @Before
+ public void before() throws NoSuchWorkspaceException, LoginException,
MalformedObjectNameException {
+ executor = Executors.newSingleThreadScheduledExecutor();
+ statsProvider = new
MetricStatisticsProvider(ManagementFactory.getPlatformMBeanServer(), executor);
+ mBeanObjectName = new ObjectName(mBeanName);
+ }
+
+ @After
+ public void after() {
+ // we have to deregister the statistics provider after each test case,
as the call to
+ // ManagementFactory.getPlatformMBeanServer() would otherwise return
the mbean server with the statistics
+ // provider from the first test case and reuse it.
+ statsProvider.close();
+ new ExecutorCloser(executor).close();
+ }
+
+ @Test
+ public void testMetricWhenAddingNodes() throws CommitFailedException,
IOException, ReflectionException,
+ InstanceNotFoundException, AttributeNotFoundException,
MBeanException, NoSuchWorkspaceException,
+ LoginException {
+ NodeStore nodeStore = new MemoryNodeStore();
+ Oak oak = getOak(nodeStore);
+ ContentSession session = oak.createContentRepository().login(null,
null);
+ Root root = session.getLatestRoot();
+ Whiteboard wb = oak.getWhiteboard();
+ setCounterIndexSeed(root, 2);
+ root.commit();
+
+ addNodes(10, 5000, root, wb); // add some random nodes and wait for
the NodeCounterIndex to appear
Review Comment:
I would add comments on their own lines (before the code)
--
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]
