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-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
