You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "liangkaiwen (via GitHub)" <gi...@apache.org> on 2025/07/01 23:34:07 UTC
[PR] [SOLR-17799] Replace segment merge metrics using gauges with consistent counters [solr]
liangkaiwen opened a new pull request, #3416:
URL: https://github.com/apache/solr/pull/3416
https://issues.apache.org/jira/browse/SOLR-XXXXX
<!--
_(If you are a project committer then you may remove some/all of the following template.)_
Before creating a pull request, please file an issue in the ASF Jira system for Solr:
* https://issues.apache.org/jira/projects/SOLR
For something minor (i.e. that wouldn't be worth putting in release notes), you can skip JIRA.
To create a Jira issue, you will need to create an account there first.
The title of the PR should reference the Jira issue number in the form:
* SOLR-####: <short description of problem or changes>
SOLR must be fully capitalized. A short description helps people scanning pull requests for items they can work on.
Properly referencing the issue in the title ensures that Jira is correctly updated with code review comments and commits. -->
# Description
Replace existing segment merge metrics with consistent counters
# Solution
* Removed the index merge running gauge metrics
* Added the following index merge metrics:
INDEX.merge.major.started.docs
INDEX.merge.major.started.deletedDocs
INDEX.merge.major.started.segments
INDEX.merge.minor.started.docs
INDEX.merge.minor.started.deletedDocs
INDEX.merge.minor.started.segments
INDEX.merge.major.completed.docs
INDEX.merge.major.completed.deletedDocs
INDEX.merge.major.completed.segments
INDEX.merge.minor.completed.docs
INDEX.merge.minor.completed.deletedDocs
INDEX.merge.minor.completed.segments
# Tests
Please describe the tests you've developed or run to confirm this patch implements the feature or solves the problem.
# Checklist
Please review the following and check all that apply:
- [ ] I have reviewed the guidelines for [How to Contribute](https://github.com/apache/solr/blob/main/CONTRIBUTING.md) and my code conforms to the standards described there to the best of my ability.
- [ ] I have created a Jira issue and added the issue ID to my pull request title.
- [ ] I have given Solr maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended, not available for branches on forks living under an organisation)
- [ ] I have developed this patch against the `main` branch.
- [ ] I have run `./gradlew check`.
- [ ] I have added tests for my changes.
- [ ] I have added documentation for the [Reference Guide](https://github.com/apache/solr/tree/main/solr/solr-ref-guide)
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] [SOLR-17799] Replace segment merge metrics using gauges with consistent counters [solr]
Posted by "liangkaiwen (via GitHub)" <gi...@apache.org>.
liangkaiwen commented on PR #3416:
URL: https://github.com/apache/solr/pull/3416#issuecomment-3025851085
Will update documentation and add tests after we confirm this is the pattern of metrics/naming that we want to use
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] [SOLR-17799] Replace segment merge metrics using gauges with consistent counters [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #3416:
URL: https://github.com/apache/solr/pull/3416#issuecomment-3034595201
+1
Nice!
I slightly prefer "finished" instead of "completed" as I think it better balances with "started", plus it's slightly shorter.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] [SOLR-17799] Replace segment merge metrics using gauges with consistent counters [solr]
Posted by "liangkaiwen (via GitHub)" <gi...@apache.org>.
liangkaiwen commented on PR #3416:
URL: https://github.com/apache/solr/pull/3416#issuecomment-3114964576
Re: backwards compatibility - this is a good point, but I don't think it would be worth to push both these new metrics and the existing ones (would be confusing having so many metrics of similar names, different types, and overlapping behaviors).
I think what I will do is shelve this implementation and instead target to have an OTEL implementation of these metrics for Solr 10 release
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] [SOLR-17799] Replace segment merge metrics using gauges with consistent counters [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley closed pull request #3416: [SOLR-17799] Replace segment merge metrics using gauges with consistent counters
URL: https://github.com/apache/solr/pull/3416
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] [SOLR-17799] Replace segment merge metrics using gauges with consistent counters [solr]
Posted by "liangkaiwen (via GitHub)" <gi...@apache.org>.
liangkaiwen commented on code in PR #3416:
URL: https://github.com/apache/solr/pull/3416#discussion_r2229633902
##########
solr/core/src/java/org/apache/solr/update/SolrIndexWriter.java:
##########
@@ -72,23 +71,33 @@ public class SolrIndexWriter extends IndexWriter {
private Directory directory;
// metrics
- private long majorMergeDocs = 512 * 1024;
- private Timer majorMerge;
- private Timer minorMerge;
- private Meter majorMergedDocs;
- private Meter majorDeletedDocs;
- private Counter mergeErrors;
+ private static final String MERGE_METRIC_PATH_COMPONENT = "merge";
+ private static final String MERGE_METRIC_PATH_COMPONENT_MAJOR = "major";
+ private static final String MERGE_METRIC_PATH_COMPONENT_MINOR = "minor";
+ private static final String MERGE_METRIC_PATH_COMPONENT_FINISHED = "finished";
+ private static final String MERGE_METRIC_PATH_COMPONENT_STARTED = "started";
+
+ private static final String MERGES_METRIC_NAME = "merges";
+ private static final String MERGE_DOCS_METRIC_NAME = "docs";
+ private static final String MERGE_DELETED_DOCS_METRIC_NAME = "deletedDocs";
+ private static final String MERGE_SEGMENTS_METRIC_NAME = "segments";
+ private static final String MERGE_TIMES_METRIC_NAME = "mergeTimes";
+ private static final String MERGE_FLUSH_METRIC_NAME = "flushes";
+ private static final String MERGE_ERRORS_METRIC_NAME = "errors";
+
+ private final Map<String, Counter> majorMergeStartedMetrics = new HashMap<>();
+ private final Map<String, Counter> minorMergeStartedMetrics = new HashMap<>();
+ private final Map<String, Counter> majorMergeFinishedMetrics = new HashMap<>();
+ private final Map<String, Counter> minorMergeFinishedMetrics = new HashMap<>();
Review Comment:
I don't know about "modified". This implementation only uses this map for get operations and never put (besides initial construction). I think with only get operations, a concurrenthashmap doesn't provide much utility no?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] [SOLR-17799] Replace segment merge metrics using gauges with consistent counters [solr]
Posted by "liangkaiwen (via GitHub)" <gi...@apache.org>.
liangkaiwen commented on code in PR #3416:
URL: https://github.com/apache/solr/pull/3416#discussion_r2222575661
##########
solr/core/src/java/org/apache/solr/update/SolrIndexWriter.java:
##########
@@ -72,23 +71,33 @@ public class SolrIndexWriter extends IndexWriter {
private Directory directory;
// metrics
- private long majorMergeDocs = 512 * 1024;
- private Timer majorMerge;
- private Timer minorMerge;
- private Meter majorMergedDocs;
- private Meter majorDeletedDocs;
- private Counter mergeErrors;
+ private static final String MERGE_METRIC_PATH_COMPONENT = "merge";
+ private static final String MERGE_METRIC_PATH_COMPONENT_MAJOR = "major";
+ private static final String MERGE_METRIC_PATH_COMPONENT_MINOR = "minor";
+ private static final String MERGE_METRIC_PATH_COMPONENT_FINISHED = "finished";
+ private static final String MERGE_METRIC_PATH_COMPONENT_STARTED = "started";
+
+ private static final String MERGES_METRIC_NAME = "merges";
+ private static final String MERGE_DOCS_METRIC_NAME = "docs";
+ private static final String MERGE_DELETED_DOCS_METRIC_NAME = "deletedDocs";
+ private static final String MERGE_SEGMENTS_METRIC_NAME = "segments";
+ private static final String MERGE_TIMES_METRIC_NAME = "mergeTimes";
+ private static final String MERGE_FLUSH_METRIC_NAME = "flushes";
+ private static final String MERGE_ERRORS_METRIC_NAME = "errors";
+
+ private static final Map<String, Counter> majorMergeStartedMetrics = new HashMap<>();
Review Comment:
you're correct, will make this a field
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] [SOLR-17799] Replace segment merge metrics using gauges with consistent counters [solr]
Posted by "mlbiscoc (via GitHub)" <gi...@apache.org>.
mlbiscoc commented on code in PR #3416:
URL: https://github.com/apache/solr/pull/3416#discussion_r2222987576
##########
solr/core/src/test/org/apache/solr/update/SolrIndexWriterTest.java:
##########
@@ -0,0 +1,171 @@
+/*
+ * 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.solr.update;
+
+import com.codahale.metrics.Counter;
+import com.codahale.metrics.Meter;
+import java.lang.invoke.MethodHandles;
+import java.util.Map;
+import org.apache.lucene.document.Document;
+import org.apache.lucene.document.Field;
+import org.apache.lucene.document.IntField;
+import org.apache.lucene.document.StringField;
+import org.apache.lucene.index.IndexWriter;
+import org.apache.lucene.index.Term;
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.core.DirectoryFactory;
+import org.apache.solr.core.SolrCore;
+import org.apache.solr.metrics.SolrMetricsContext;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * SolrIndexWriter is pretty much identical to Lucene's IndexWriter so these tests are only for
+ * testing metrics collection
+ */
+public class SolrIndexWriterTest extends SolrTestCaseJ4 {
+ private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ IndexWriter iw;
+ SolrMetricsContext solrMetricsContext;
+
+ @BeforeClass
+ public static void beforeClass() throws Exception {
+ System.setProperty("solr.directoryFactory", "solr.NRTCachingDirectoryFactory");
+ System.setProperty("solr.tests.lockType", DirectoryFactory.LOCK_TYPE_SIMPLE);
+
+ initCore("solrconfig.xml", "schema15.xml");
+ }
+
+ @Override
+ @Before
+ public void setUp() throws Exception {
+ super.setUp();
+ clearIndex();
+ SolrCore core = h.getCore();
+ iw = core.getSolrCoreState().getIndexWriter(core).get();
+ ((SolrIndexWriter) iw).setMajorMergeDocs(SolrIndexWriter.DEFAULT_MAJOR_MERGE_DOCS);
+ solrMetricsContext = core.getSolrMetricsContext();
+ }
+
+ @Test
+ public void testMinorMergeMetrics() throws Exception {
+ Map<String, Counter> metrics = solrMetricsContext.getMetricRegistry().getCounters();
+
+ // we are intentionally only testing "started" metrics because it is difficult to block until
+ // merge fully completes
+ Counter minorMergeStartedMetrics = metrics.get("INDEX.merge.minor.started.merges");
+ Counter minorMergeStartedDocs = metrics.get("INDEX.merge.minor.started.docs");
+ Counter minorMergeStartedSegments = metrics.get("INDEX.merge.minor.started.segments");
+ long initialMerges = minorMergeStartedMetrics.getCount();
+ long initialDocs = minorMergeStartedDocs.getCount();
+ long initialSegments = minorMergeStartedSegments.getCount();
+
+ for (int i = 1; i <= 100; i++) {
+ Document doc = new Document();
+ doc.add(new StringField("id", String.valueOf(i), Field.Store.YES));
+ doc.add(new IntField("number", i, Field.Store.YES));
+ iw.addDocument(doc);
+ if (i % 10 == 0) iw.commit(); // make a new segment every 10th document
+ }
+ iw.forceMerge(1, true);
+
+ assertEquals(
+ "should be single merge operation", initialMerges + 1, minorMergeStartedMetrics.getCount());
+ assertEquals("should merge all documents", initialDocs + 100, minorMergeStartedDocs.getCount());
+ assertEquals(
+ "should merge all segments", initialSegments + 10, minorMergeStartedSegments.getCount());
+ }
+
+ @Test
+ public void testMinorMergeMetrics_deletedDocs() throws Exception {
+ Map<String, Counter> metrics = solrMetricsContext.getMetricRegistry().getCounters();
+
+ // we are intentionally only testing "started" metrics because it is difficult to block until
+ // merge fully completes
+ Counter minorMergeStartedMetrics = metrics.get("INDEX.merge.minor.started.merges");
+ Counter minorMergeStartedDocs = metrics.get("INDEX.merge.minor.started.docs");
+ Counter minorMergeStartedSegments = metrics.get("INDEX.merge.minor.started.segments");
+ Counter minorMergeStartedDeletedDocs = metrics.get("INDEX.merge.minor.started.deletedDocs");
+ long initialMerges = minorMergeStartedMetrics.getCount();
+ long initialDocs = minorMergeStartedDocs.getCount();
+ long initialSegments = minorMergeStartedSegments.getCount();
+ long initialDeletedDocs = minorMergeStartedDeletedDocs.getCount();
+
+ for (int i = 1; i <= 100; i++) {
+ Document doc = new Document();
+ doc.add(new StringField("id", String.valueOf(i), Field.Store.YES));
+ doc.add(new IntField("number", i, Field.Store.YES));
+ iw.addDocument(doc);
+ if (i % 10 == 0) iw.commit(); // make a new segment every 10th document
+ }
+ for (int i = 1; i <= 3; i++) {
+ iw.deleteDocuments(new Term("id", String.valueOf(i)));
+ }
+ iw.commit();
+ iw.forceMerge(1, true);
+
+ assertEquals(
+ "should be mulitple merge operations",
+ initialMerges + 1,
+ minorMergeStartedMetrics.getCount());
Review Comment:
Isn't this technically just 1 merge from line 123? Why would initial be higher than 0? Is it because minor merges on itself with less docs?
##########
solr/core/src/java/org/apache/solr/update/SolrIndexWriter.java:
##########
@@ -269,54 +204,22 @@ protected void merge(MergePolicy.OneMerge merge) throws IOException {
String segString = merge.segString();
long totalNumDocs = merge.totalNumDocs();
runningMerges.put(segString, totalNumDocs);
- if (!mergeTotals) {
- try {
- super.merge(merge);
- } finally {
- runningMerges.remove(segString);
- }
- return;
- }
long deletedDocs = 0;
for (SegmentCommitInfo info : merge.segments) {
totalNumDocs -= info.getDelCount();
deletedDocs += info.getDelCount();
}
- boolean major = totalNumDocs > majorMergeDocs;
int segmentsCount = merge.segments.size();
- Timer.Context context;
- if (major) {
- runningMajorMerges.incrementAndGet();
- runningMajorMergesDocs.addAndGet(totalNumDocs);
- runningMajorMergesSegments.addAndGet(segmentsCount);
- if (mergeDetails) {
- majorMergedDocs.mark(totalNumDocs);
- majorDeletedDocs.mark(deletedDocs);
- }
- context = majorMerge.time();
- } else {
- runningMinorMerges.incrementAndGet();
- runningMinorMergesDocs.addAndGet(totalNumDocs);
- runningMinorMergesSegments.addAndGet(segmentsCount);
- context = minorMerge.time();
- }
+ Timer.Context context =
+ updateMergeMetrics(totalNumDocs, deletedDocs, segmentsCount, false, null);
try {
super.merge(merge);
} catch (Throwable t) {
- mergeErrors.inc();
+ mergeErrorsCounter.inc();
throw t;
} finally {
runningMerges.remove(segString);
- context.stop();
- if (major) {
- runningMajorMerges.decrementAndGet();
- runningMajorMergesDocs.addAndGet(-totalNumDocs);
- runningMajorMergesSegments.addAndGet(-segmentsCount);
- } else {
- runningMinorMerges.decrementAndGet();
- runningMinorMergesDocs.addAndGet(-totalNumDocs);
- runningMinorMergesSegments.addAndGet(-segmentsCount);
- }
+ updateMergeMetrics(totalNumDocs, deletedDocs, segmentsCount, true, context);
Review Comment:
Should this be negative `totalNumDocs` and `-segmentsCount`?
##########
solr/core/src/java/org/apache/solr/update/SolrIndexWriter.java:
##########
Review Comment:
A bit surprised this isn't implementer of `SolrMetricProducer` and all metrics are initialized in the constructor. Maybe the `SolrMetricProducer` was create later on and this never moved. I think we should move this to implement that interface which basically does your `initMetrics()`.
##########
solr/core/src/java/org/apache/solr/update/SolrIndexWriter.java:
##########
@@ -72,23 +71,33 @@ public class SolrIndexWriter extends IndexWriter {
private Directory directory;
// metrics
- private long majorMergeDocs = 512 * 1024;
- private Timer majorMerge;
- private Timer minorMerge;
- private Meter majorMergedDocs;
- private Meter majorDeletedDocs;
- private Counter mergeErrors;
+ private static final String MERGE_METRIC_PATH_COMPONENT = "merge";
+ private static final String MERGE_METRIC_PATH_COMPONENT_MAJOR = "major";
+ private static final String MERGE_METRIC_PATH_COMPONENT_MINOR = "minor";
+ private static final String MERGE_METRIC_PATH_COMPONENT_FINISHED = "finished";
+ private static final String MERGE_METRIC_PATH_COMPONENT_STARTED = "started";
+
+ private static final String MERGES_METRIC_NAME = "merges";
+ private static final String MERGE_DOCS_METRIC_NAME = "docs";
+ private static final String MERGE_DELETED_DOCS_METRIC_NAME = "deletedDocs";
+ private static final String MERGE_SEGMENTS_METRIC_NAME = "segments";
+ private static final String MERGE_TIMES_METRIC_NAME = "mergeTimes";
+ private static final String MERGE_FLUSH_METRIC_NAME = "flushes";
+ private static final String MERGE_ERRORS_METRIC_NAME = "errors";
+
+ private final Map<String, Counter> majorMergeStartedMetrics = new HashMap<>();
+ private final Map<String, Counter> minorMergeStartedMetrics = new HashMap<>();
+ private final Map<String, Counter> majorMergeFinishedMetrics = new HashMap<>();
+ private final Map<String, Counter> minorMergeFinishedMetrics = new HashMap<>();
Review Comment:
I see you are updating these with `updateMergeMetrics`. But this isn't thread safe as these can be modified concurrently from multiple threads from a merge. This should probably be a `ConcurrentHashMap`?
##########
solr/core/src/java/org/apache/solr/update/SolrIndexWriter.java:
##########
@@ -72,23 +71,33 @@ public class SolrIndexWriter extends IndexWriter {
private Directory directory;
// metrics
- private long majorMergeDocs = 512 * 1024;
- private Timer majorMerge;
- private Timer minorMerge;
- private Meter majorMergedDocs;
- private Meter majorDeletedDocs;
- private Counter mergeErrors;
+ private static final String MERGE_METRIC_PATH_COMPONENT = "merge";
+ private static final String MERGE_METRIC_PATH_COMPONENT_MAJOR = "major";
+ private static final String MERGE_METRIC_PATH_COMPONENT_MINOR = "minor";
+ private static final String MERGE_METRIC_PATH_COMPONENT_FINISHED = "finished";
+ private static final String MERGE_METRIC_PATH_COMPONENT_STARTED = "started";
+
+ private static final String MERGES_METRIC_NAME = "merges";
+ private static final String MERGE_DOCS_METRIC_NAME = "docs";
+ private static final String MERGE_DELETED_DOCS_METRIC_NAME = "deletedDocs";
+ private static final String MERGE_SEGMENTS_METRIC_NAME = "segments";
+ private static final String MERGE_TIMES_METRIC_NAME = "mergeTimes";
+ private static final String MERGE_FLUSH_METRIC_NAME = "flushes";
+ private static final String MERGE_ERRORS_METRIC_NAME = "errors";
Review Comment:
I'm usually a fan of constant static strings but in this case there are so many, I think I preferred it the other way of just in-lining them as I found it easier to read that it went something like
```
solrMetricsContext.gauge(
() -> runningMajorMerges.get(),
true,
"running",
SolrInfoBean.Category.INDEX.toString(),
"merge",
"major");
```
`running.index.merge.major`. I don't feel strongly about it though so if you prefer this way thats fine I guess.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] [SOLR-17799] Replace segment merge metrics using gauges with consistent counters [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #3416:
URL: https://github.com/apache/solr/pull/3416#issuecomment-3398810908
Closing, as the work continued in #3682
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] [SOLR-17799] Replace segment merge metrics using gauges with consistent counters [solr]
Posted by "liangkaiwen (via GitHub)" <gi...@apache.org>.
liangkaiwen commented on code in PR #3416:
URL: https://github.com/apache/solr/pull/3416#discussion_r2229636794
##########
solr/core/src/java/org/apache/solr/update/SolrIndexWriter.java:
##########
@@ -269,54 +204,22 @@ protected void merge(MergePolicy.OneMerge merge) throws IOException {
String segString = merge.segString();
long totalNumDocs = merge.totalNumDocs();
runningMerges.put(segString, totalNumDocs);
- if (!mergeTotals) {
- try {
- super.merge(merge);
- } finally {
- runningMerges.remove(segString);
- }
- return;
- }
long deletedDocs = 0;
for (SegmentCommitInfo info : merge.segments) {
totalNumDocs -= info.getDelCount();
deletedDocs += info.getDelCount();
}
- boolean major = totalNumDocs > majorMergeDocs;
int segmentsCount = merge.segments.size();
- Timer.Context context;
- if (major) {
- runningMajorMerges.incrementAndGet();
- runningMajorMergesDocs.addAndGet(totalNumDocs);
- runningMajorMergesSegments.addAndGet(segmentsCount);
- if (mergeDetails) {
- majorMergedDocs.mark(totalNumDocs);
- majorDeletedDocs.mark(deletedDocs);
- }
- context = majorMerge.time();
- } else {
- runningMinorMerges.incrementAndGet();
- runningMinorMergesDocs.addAndGet(totalNumDocs);
- runningMinorMergesSegments.addAndGet(segmentsCount);
- context = minorMerge.time();
- }
+ Timer.Context context =
+ updateMergeMetrics(totalNumDocs, deletedDocs, segmentsCount, false, null);
try {
super.merge(merge);
} catch (Throwable t) {
- mergeErrors.inc();
+ mergeErrorsCounter.inc();
throw t;
} finally {
runningMerges.remove(segString);
- context.stop();
- if (major) {
- runningMajorMerges.decrementAndGet();
- runningMajorMergesDocs.addAndGet(-totalNumDocs);
- runningMajorMergesSegments.addAndGet(-segmentsCount);
- } else {
- runningMinorMerges.decrementAndGet();
- runningMinorMergesDocs.addAndGet(-totalNumDocs);
- runningMinorMergesSegments.addAndGet(-segmentsCount);
- }
+ updateMergeMetrics(totalNumDocs, deletedDocs, segmentsCount, true, context);
Review Comment:
No, it shouldn't be negative because this implementation no longer tracks the state of a current merge. The previous implementation uses negatives because the merge has finished therefore there are "no documents being currently merged"
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] [SOLR-17799] Replace segment merge metrics using gauges with consistent counters [solr]
Posted by "liangkaiwen (via GitHub)" <gi...@apache.org>.
liangkaiwen commented on code in PR #3416:
URL: https://github.com/apache/solr/pull/3416#discussion_r2229723814
##########
solr/core/src/test/org/apache/solr/update/SolrIndexWriterTest.java:
##########
@@ -0,0 +1,171 @@
+/*
+ * 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.solr.update;
+
+import com.codahale.metrics.Counter;
+import com.codahale.metrics.Meter;
+import java.lang.invoke.MethodHandles;
+import java.util.Map;
+import org.apache.lucene.document.Document;
+import org.apache.lucene.document.Field;
+import org.apache.lucene.document.IntField;
+import org.apache.lucene.document.StringField;
+import org.apache.lucene.index.IndexWriter;
+import org.apache.lucene.index.Term;
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.core.DirectoryFactory;
+import org.apache.solr.core.SolrCore;
+import org.apache.solr.metrics.SolrMetricsContext;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * SolrIndexWriter is pretty much identical to Lucene's IndexWriter so these tests are only for
+ * testing metrics collection
+ */
+public class SolrIndexWriterTest extends SolrTestCaseJ4 {
+ private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ IndexWriter iw;
+ SolrMetricsContext solrMetricsContext;
+
+ @BeforeClass
+ public static void beforeClass() throws Exception {
+ System.setProperty("solr.directoryFactory", "solr.NRTCachingDirectoryFactory");
+ System.setProperty("solr.tests.lockType", DirectoryFactory.LOCK_TYPE_SIMPLE);
+
+ initCore("solrconfig.xml", "schema15.xml");
+ }
+
+ @Override
+ @Before
+ public void setUp() throws Exception {
+ super.setUp();
+ clearIndex();
+ SolrCore core = h.getCore();
+ iw = core.getSolrCoreState().getIndexWriter(core).get();
+ ((SolrIndexWriter) iw).setMajorMergeDocs(SolrIndexWriter.DEFAULT_MAJOR_MERGE_DOCS);
+ solrMetricsContext = core.getSolrMetricsContext();
+ }
+
+ @Test
+ public void testMinorMergeMetrics() throws Exception {
+ Map<String, Counter> metrics = solrMetricsContext.getMetricRegistry().getCounters();
+
+ // we are intentionally only testing "started" metrics because it is difficult to block until
+ // merge fully completes
+ Counter minorMergeStartedMetrics = metrics.get("INDEX.merge.minor.started.merges");
+ Counter minorMergeStartedDocs = metrics.get("INDEX.merge.minor.started.docs");
+ Counter minorMergeStartedSegments = metrics.get("INDEX.merge.minor.started.segments");
+ long initialMerges = minorMergeStartedMetrics.getCount();
+ long initialDocs = minorMergeStartedDocs.getCount();
+ long initialSegments = minorMergeStartedSegments.getCount();
+
+ for (int i = 1; i <= 100; i++) {
+ Document doc = new Document();
+ doc.add(new StringField("id", String.valueOf(i), Field.Store.YES));
+ doc.add(new IntField("number", i, Field.Store.YES));
+ iw.addDocument(doc);
+ if (i % 10 == 0) iw.commit(); // make a new segment every 10th document
+ }
+ iw.forceMerge(1, true);
+
+ assertEquals(
+ "should be single merge operation", initialMerges + 1, minorMergeStartedMetrics.getCount());
+ assertEquals("should merge all documents", initialDocs + 100, minorMergeStartedDocs.getCount());
+ assertEquals(
+ "should merge all segments", initialSegments + 10, minorMergeStartedSegments.getCount());
+ }
+
+ @Test
+ public void testMinorMergeMetrics_deletedDocs() throws Exception {
+ Map<String, Counter> metrics = solrMetricsContext.getMetricRegistry().getCounters();
+
+ // we are intentionally only testing "started" metrics because it is difficult to block until
+ // merge fully completes
+ Counter minorMergeStartedMetrics = metrics.get("INDEX.merge.minor.started.merges");
+ Counter minorMergeStartedDocs = metrics.get("INDEX.merge.minor.started.docs");
+ Counter minorMergeStartedSegments = metrics.get("INDEX.merge.minor.started.segments");
+ Counter minorMergeStartedDeletedDocs = metrics.get("INDEX.merge.minor.started.deletedDocs");
+ long initialMerges = minorMergeStartedMetrics.getCount();
+ long initialDocs = minorMergeStartedDocs.getCount();
+ long initialSegments = minorMergeStartedSegments.getCount();
+ long initialDeletedDocs = minorMergeStartedDeletedDocs.getCount();
+
+ for (int i = 1; i <= 100; i++) {
+ Document doc = new Document();
+ doc.add(new StringField("id", String.valueOf(i), Field.Store.YES));
+ doc.add(new IntField("number", i, Field.Store.YES));
+ iw.addDocument(doc);
+ if (i % 10 == 0) iw.commit(); // make a new segment every 10th document
+ }
+ for (int i = 1; i <= 3; i++) {
+ iw.deleteDocuments(new Term("id", String.valueOf(i)));
+ }
+ iw.commit();
+ iw.forceMerge(1, true);
+
+ assertEquals(
+ "should be mulitple merge operations",
+ initialMerges + 1,
+ minorMergeStartedMetrics.getCount());
Review Comment:
forgot to update the assertion message. You're right, should be single
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] [SOLR-17799] Replace segment merge metrics using gauges with consistent counters [solr]
Posted by "psalagnac (via GitHub)" <gi...@apache.org>.
psalagnac commented on code in PR #3416:
URL: https://github.com/apache/solr/pull/3416#discussion_r2221771781
##########
solr/core/src/java/org/apache/solr/update/SolrIndexWriter.java:
##########
@@ -72,23 +71,33 @@ public class SolrIndexWriter extends IndexWriter {
private Directory directory;
// metrics
- private long majorMergeDocs = 512 * 1024;
- private Timer majorMerge;
- private Timer minorMerge;
- private Meter majorMergedDocs;
- private Meter majorDeletedDocs;
- private Counter mergeErrors;
+ private static final String MERGE_METRIC_PATH_COMPONENT = "merge";
+ private static final String MERGE_METRIC_PATH_COMPONENT_MAJOR = "major";
+ private static final String MERGE_METRIC_PATH_COMPONENT_MINOR = "minor";
+ private static final String MERGE_METRIC_PATH_COMPONENT_FINISHED = "finished";
+ private static final String MERGE_METRIC_PATH_COMPONENT_STARTED = "started";
+
+ private static final String MERGES_METRIC_NAME = "merges";
+ private static final String MERGE_DOCS_METRIC_NAME = "docs";
+ private static final String MERGE_DELETED_DOCS_METRIC_NAME = "deletedDocs";
+ private static final String MERGE_SEGMENTS_METRIC_NAME = "segments";
+ private static final String MERGE_TIMES_METRIC_NAME = "mergeTimes";
+ private static final String MERGE_FLUSH_METRIC_NAME = "flushes";
+ private static final String MERGE_ERRORS_METRIC_NAME = "errors";
+
+ private static final Map<String, Counter> majorMergeStartedMetrics = new HashMap<>();
Review Comment:
I'm confused by these static (global) maps. Shouldn't we have per core metrics?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] [SOLR-17799] Replace segment merge metrics using gauges with consistent counters [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #3416:
URL: https://github.com/apache/solr/pull/3416#issuecomment-3104833821
Just want to react to something:
> otherwise we run the risk of blowing up cardinality for clouds with many cores without them knowing the moment they call the /admin/metrics endpoint without filtering.
This is already a risk. Any real-world use of fetching /admin/metrics is going to have a filtering parameter. It'd be reasonable to detect that no filter was given and then potentially return an error. If the user is just kicking the tires, they may want to ignore the filter requirement so the error message might contain a trivial suggestion like to see all Solr metrics.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] [SOLR-17799] Replace segment merge metrics using gauges with consistent counters [solr]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #3416:
URL: https://github.com/apache/solr/pull/3416#issuecomment-3321952319
This PR has had no activity for 60 days and is now labeled as stale. Any new activity will remove the stale label. To attract more reviewers, please tag people who might be familiar with the code area and/or notify the dev@solr.apache.org mailing list. To exempt this PR from being marked as stale, make it a draft PR or add the label "exempt-stale". If left unattended, this PR will be closed after another 60 days of inactivity. Thank you for your contribution!
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org