You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jackrabbit.apache.org by "steffenvan (via GitHub)" <gi...@apache.org> on 2023/04/27 10:15:37 UTC
[GitHub] [jackrabbit-oak] steffenvan opened a new pull request, #914: OAK-10214: Expose node counter value as a metric in Oak
steffenvan opened a new pull request, #914:
URL: https://github.com/apache/jackrabbit-oak/pull/914
Adding a metric for the node counter.
--
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-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] thomasmueller commented on a diff in pull request #914: OAK-10214: Expose node counter value as a metric in Oak
Posted by "thomasmueller (via GitHub)" <gi...@apache.org>.
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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] steffenvan commented on a diff in pull request #914: OAK-10214: Expose node counter value as a metric in Oak
Posted by "steffenvan (via GitHub)" <gi...@apache.org>.
steffenvan commented on code in PR #914:
URL: https://github.com/apache/jackrabbit-oak/pull/914#discussion_r1187125687
##########
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:
Done
--
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-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] steffenvan commented on a diff in pull request #914: OAK-10214: Expose node counter value as a metric in Oak
Posted by "steffenvan (via GitHub)" <gi...@apache.org>.
steffenvan commented on code in PR #914:
URL: https://github.com/apache/jackrabbit-oak/pull/914#discussion_r1185814049
##########
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:
Noted for the future!
--
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-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] thomasmueller merged pull request #914: OAK-10214: Expose node counter value as a metric in Oak
Posted by "thomasmueller (via GitHub)" <gi...@apache.org>.
thomasmueller merged PR #914:
URL: https://github.com/apache/jackrabbit-oak/pull/914
--
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-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org