cpoerschke commented on code in PR #2382: URL: https://github.com/apache/solr/pull/2382#discussion_r1572044870
########## solr/modules/monitor/src/java/org/apache/solr/monitor/update/MonitorUpdateProcessorFactory.java: ########## @@ -0,0 +1,56 @@ +/* + * + * * 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.monitor.update; + +import org.apache.lucene.monitor.MonitorFields; +import org.apache.lucene.monitor.Presearcher; +import org.apache.solr.common.util.NamedList; +import org.apache.solr.monitor.PresearcherFactory; +import org.apache.solr.request.SolrQueryRequest; +import org.apache.solr.response.SolrQueryResponse; +import org.apache.solr.update.processor.UpdateRequestProcessor; +import org.apache.solr.update.processor.UpdateRequestProcessorFactory; + +public class MonitorUpdateProcessorFactory extends UpdateRequestProcessorFactory { + + private Presearcher presearcher = PresearcherFactory.build(); + private String queryFieldNameOverride; + private String payloadFieldNameOverride; Review Comment: subjective: could maybe initialise to the defaults here, overriding in `init` if applicable, and then avoid the `null-or-not` checks in `getInstance` ```suggestion private String queryFieldName = MonitorFields.MONITOR_QUERY ; private String payloadFieldName = MonitorFields.PAYLOAD; ``` ########## solr/modules/monitor/src/java/org/apache/solr/monitor/update/MonitorUpdateProcessorFactory.java: ########## @@ -0,0 +1,56 @@ +/* + * + * * 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.monitor.update; + +import org.apache.lucene.monitor.MonitorFields; +import org.apache.lucene.monitor.Presearcher; +import org.apache.solr.common.util.NamedList; +import org.apache.solr.monitor.PresearcherFactory; +import org.apache.solr.request.SolrQueryRequest; +import org.apache.solr.response.SolrQueryResponse; +import org.apache.solr.update.processor.UpdateRequestProcessor; +import org.apache.solr.update.processor.UpdateRequestProcessorFactory; + +public class MonitorUpdateProcessorFactory extends UpdateRequestProcessorFactory { + + private Presearcher presearcher = PresearcherFactory.build(); Review Comment: noting that `init` has `PresearcherFactory.build(presearcherType)` also. ```suggestion private Presearcher presearcher; ``` ########## solr/modules/monitor/src/java/org/apache/solr/monitor/search/ReverseSearchComponent.java: ########## @@ -0,0 +1,197 @@ +/* + * + * * 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.monitor.search; + +import static org.apache.solr.monitor.MonitorConstants.DOCUMENT_BATCH_KEY; +import static org.apache.solr.monitor.MonitorConstants.MONITOR_DOCUMENTS_KEY; +import static org.apache.solr.monitor.MonitorConstants.MONITOR_OUTPUT_KEY; +import static org.apache.solr.monitor.MonitorConstants.QUERY_MATCH_TYPE_KEY; +import static org.apache.solr.monitor.MonitorConstants.REVERSE_SEARCH_PARAM_NAME; +import static org.apache.solr.monitor.MonitorConstants.SOLR_MONITOR_CACHE_NAME; +import static org.apache.solr.monitor.MonitorConstants.WRITE_TO_DOC_LIST_KEY; +import static org.apache.solr.monitor.search.QueryMatchResponseCodec.mergeResponses; + +import java.io.IOException; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.concurrent.ExecutorService; +import java.util.stream.Collectors; +import org.apache.lucene.document.Document; +import org.apache.lucene.monitor.DocumentBatchVisitor; +import org.apache.lucene.search.MatchAllDocsQuery; +import org.apache.lucene.search.Query; +import org.apache.lucene.util.NamedThreadFactory; +import org.apache.solr.common.SolrException; +import org.apache.solr.common.SolrException.ErrorCode; +import org.apache.solr.common.util.ExecutorUtil; +import org.apache.solr.common.util.NamedList; +import org.apache.solr.core.CloseHook; +import org.apache.solr.core.SolrCore; +import org.apache.solr.handler.component.ResponseBuilder; +import org.apache.solr.handler.component.SearchComponent; +import org.apache.solr.handler.component.ShardRequest; +import org.apache.solr.handler.loader.JsonLoader; +import org.apache.solr.monitor.SolrMonitorQueryDecoder; +import org.apache.solr.monitor.cache.MonitorQueryCache; +import org.apache.solr.monitor.cache.SharedMonitorCache; +import org.apache.solr.request.SolrQueryRequest; +import org.apache.solr.search.QParser; +import org.apache.solr.search.SyntaxError; +import org.apache.solr.update.DocumentBuilder; +import org.apache.solr.util.plugin.SolrCoreAware; + +public class ReverseSearchComponent extends SearchComponent implements SolrCoreAware { + + private static final String MATCHER_THREAD_COUNT_KEY = "threadCount"; + + private SolrMatcherSinkFactory solrMatcherSinkFactory = new SolrMatcherSinkFactory(); + + private ExecutorService executor; + + @Override + public void init(NamedList<?> args) { + super.init(args); + Object threadCount = args.get(MATCHER_THREAD_COUNT_KEY); + if (threadCount instanceof Integer) { + executor = + ExecutorUtil.newMDCAwareFixedThreadPool( + (Integer) threadCount, new NamedThreadFactory("monitor-matcher")); + solrMatcherSinkFactory = new SolrMatcherSinkFactory(executor); + } + } + + @Override + public void prepare(ResponseBuilder rb) { + if (skipReverseSearch(rb)) { + return; + } + var req = rb.req; + var documentBatch = documentBatch(req); + req.getContext().put(DOCUMENT_BATCH_KEY, documentBatch); + var writeToDocListRaw = req.getParams().get(WRITE_TO_DOC_LIST_KEY, "false"); + boolean writeToDocList = Boolean.parseBoolean(writeToDocListRaw); + var matchType = QueryMatchType.fromString(req.getParams().get(QUERY_MATCH_TYPE_KEY)); + Map<String, Object> monitorResult = new HashMap<>(); + var matcherSink = solrMatcherSinkFactory.build(matchType, documentBatch, monitorResult); + if (matcherSink.isParallel() && writeToDocList) { + throw new SolrException( + ErrorCode.BAD_REQUEST, + "writeToDocList is not supported for parallel matcher. Consider adding more shards/cores instead of parallel matcher."); + } + try { + Query preFilterQuery = + QParser.getParser("{!" + ReverseQueryParserPlugin.NAME + "}", req).parse(); + List<Query> mutableFilters = + Optional.ofNullable(rb.getFilters()).map(ArrayList::new).orElseGet(ArrayList::new); + rb.setQuery(new MatchAllDocsQuery()); + mutableFilters.add(preFilterQuery); + var searcher = req.getSearcher(); + MonitorQueryCache solrMonitorCache = + (SharedMonitorCache) searcher.getCache(SOLR_MONITOR_CACHE_NAME); + SolrMonitorQueryDecoder queryDecoder = SolrMonitorQueryDecoder.fromCore(req.getCore()); + mutableFilters.add( + new MonitorPostFilter( + new SolrMonitorQueryCollector.CollectorContext( + solrMonitorCache, queryDecoder, matcherSink, writeToDocList, matchType))); + rb.setFilters(mutableFilters); + rb.rsp.add(QUERY_MATCH_TYPE_KEY, matchType.name()); + if (matchType != QueryMatchType.NONE) { + rb.rsp.add(MONITOR_OUTPUT_KEY, monitorResult); + } + } catch (SyntaxError e) { + throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "Syntax error in query: ", e); + } + } + + @SuppressWarnings({"unchecked"}) + private DocumentBatchVisitor documentBatch(SolrQueryRequest req) { + Object jsonParams = req.getJSON().get("params"); + if (!(jsonParams instanceof Map)) { + throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "need params"); + } + var paramMap = (Map<?, ?>) jsonParams; + var documents = paramMap.get(MONITOR_DOCUMENTS_KEY); + if (!(documents instanceof List)) { + throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "need documents list"); + } + List<Document> luceneDocs = new ArrayList<>(); + for (var document : (List<?>) documents) { + if (!(document instanceof Map) + || !((Map<?, ?>) document).keySet().stream().allMatch(key -> key instanceof String)) { + throw new SolrException( + SolrException.ErrorCode.BAD_REQUEST, "document needs to be a string-keyed map"); + } + var solrInputDoc = JsonLoader.buildDoc((Map<String, Object>) document); + var luceneDoc = DocumentBuilder.toDocument(solrInputDoc, req.getSchema()); + luceneDocs.add(luceneDoc); + } + return DocumentBatchVisitor.of(req.getSchema().getIndexAnalyzer(), luceneDocs); + } + + @Override + public void process(ResponseBuilder rb) throws IOException { + /* no-op */ + } + + @Override + public void handleResponses(ResponseBuilder rb, ShardRequest sreq) { + if (skipReverseSearch(rb) || (sreq.purpose & ShardRequest.PURPOSE_GET_TOP_IDS) == 0) { + return; + } + var responses = + sreq.responses.stream() + .map(shardResponse -> shardResponse.getSolrResponse().getResponse()) + .collect(Collectors.toList()); + rb.rsp.getValues().removeAll(MONITOR_OUTPUT_KEY); + var matchType = QueryMatchType.fromString(rb.req.getParams().get(QUERY_MATCH_TYPE_KEY)); + if (matchType != QueryMatchType.NONE) { + var finalOutput = mergeResponses(responses, matchType); + rb.rsp.add(MONITOR_OUTPUT_KEY, finalOutput); + } + } + + @Override + public void finishStage(ResponseBuilder rb) { + super.finishStage(rb); + } Review Comment: ```suggestion ``` ########## solr/modules/monitor/src/java/org/apache/solr/monitor/search/ReverseQueryParserPlugin.java: ########## @@ -0,0 +1,53 @@ +/* + * + * * 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.monitor.search; + +import java.io.IOException; +import org.apache.lucene.monitor.Presearcher; +import org.apache.solr.common.params.SolrParams; +import org.apache.solr.common.util.NamedList; +import org.apache.solr.monitor.PresearcherFactory; +import org.apache.solr.request.SolrQueryRequest; +import org.apache.solr.search.QParser; +import org.apache.solr.search.QParserPlugin; + +public class ReverseQueryParserPlugin extends QParserPlugin { + + public static final String NAME = "reverse"; + + // TODO parameterize this + private Presearcher presearcher = PresearcherFactory.build(PresearcherFactory.TERM_FILTERED); + + @Override + public QParser createParser( + String qstr, SolrParams localParams, SolrParams params, SolrQueryRequest req) { + return new ReverseQueryParser(qstr, localParams, params, req, presearcher); + } + + @Override + public void close() throws IOException { + super.close(); + } + + @Override + public void init(NamedList<?> args) { + super.init(args); + } Review Comment: ```suggestion ``` ########## solr/modules/monitor/src/java/org/apache/solr/monitor/package-info.java: ########## @@ -0,0 +1,21 @@ +/* + * + * * 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. + * + */ + +/** This package contains Solr's lucene monitor integration. */ +package org.apache.solr.monitor; Review Comment: minor: surprised that `package-info.java` seems to be not needed for the `lucene/monitor` sub-directory, or maybe the checking logic just isn't checking for it. ########## solr/modules/monitor/src/java/org/apache/solr/monitor/update/MonitorUpdateRequestProcessor.java: ########## @@ -0,0 +1,265 @@ +/* + * + * * 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.monitor.update; + +import java.io.IOException; +import java.io.Reader; +import java.util.ArrayList; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.stream.Collectors; +import java.util.stream.Stream; +import org.apache.lucene.analysis.Analyzer; +import org.apache.lucene.analysis.TokenStream; +import org.apache.lucene.document.Document; +import org.apache.lucene.document.Field; +import org.apache.lucene.document.InvertableType; +import org.apache.lucene.document.StoredValue; +import org.apache.lucene.index.DocValuesType; +import org.apache.lucene.index.IndexableField; +import org.apache.lucene.index.IndexableFieldType; +import org.apache.lucene.monitor.MonitorFields; +import org.apache.lucene.monitor.MonitorQuery; +import org.apache.lucene.monitor.Presearcher; +import org.apache.lucene.monitor.QCEVisitor; +import org.apache.lucene.util.BytesRef; +import org.apache.solr.common.SolrException; +import org.apache.solr.common.SolrInputDocument; +import org.apache.solr.common.util.JavaBinCodec; +import org.apache.solr.core.SolrCore; +import org.apache.solr.monitor.MonitorConstants; +import org.apache.solr.monitor.MonitorSchemaFields; +import org.apache.solr.monitor.SimpleQueryParser; +import org.apache.solr.schema.IndexSchema; +import org.apache.solr.schema.SchemaField; +import org.apache.solr.update.AddUpdateCommand; +import org.apache.solr.update.processor.UpdateRequestProcessor; + +public class MonitorUpdateRequestProcessor extends UpdateRequestProcessor { + + private final String queryFieldName; + private final String payloadFieldName; + private final SolrCore core; + private final IndexSchema indexSchema; + private final Presearcher presearcher; + private final MonitorSchemaFields monitorSchemaFields; + + public MonitorUpdateRequestProcessor( + UpdateRequestProcessor next, + String queryFieldName, + SolrCore core, + Presearcher presearcher, + String payloadFieldName) { + super(next); + this.queryFieldName = queryFieldName; + this.payloadFieldName = payloadFieldName; + this.core = core; + this.indexSchema = core.getLatestSchema(); Review Comment: Wondering if there's an assumption somehow w.r.t. `queryFieldName` and `payloadFieldName` being or not being within `indexSchema` -- and if there's an assumption to check it somewhere, maybe at initialisation time rather than when the first document(s) arrive to make use of the fields etc. Likewise w.r.t. the `MonitorFields.RESERVED_MONITOR_FIELDS` referenced later on. ########## solr/modules/monitor/src/java/org/apache/solr/monitor/search/ReverseQueryParser.java: ########## @@ -0,0 +1,74 @@ +/* + * + * * 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.monitor.search; + +import static org.apache.solr.monitor.MonitorConstants.DOCUMENT_BATCH_KEY; +import static org.apache.solr.monitor.MonitorConstants.SOLR_MONITOR_CACHE_NAME; + +import java.util.function.BiPredicate; +import org.apache.lucene.index.LeafReader; +import org.apache.lucene.monitor.DocumentBatchVisitor; +import org.apache.lucene.monitor.Presearcher; +import org.apache.lucene.search.Query; +import org.apache.lucene.util.BytesRef; +import org.apache.solr.common.params.SolrParams; +import org.apache.solr.monitor.cache.MonitorQueryCache; +import org.apache.solr.request.SolrQueryRequest; +import org.apache.solr.search.QParser; +import org.apache.solr.search.SyntaxError; + +public class ReverseQueryParser extends QParser { + + private final Presearcher presearcher; + + public ReverseQueryParser( + String qstr, + SolrParams localParams, + SolrParams params, + SolrQueryRequest req, + Presearcher presearcher) { + super(qstr, localParams, params, req); + this.presearcher = presearcher; + } + + @Override + public Query parse() throws SyntaxError { + var obj = req.getContext().get(DOCUMENT_BATCH_KEY); + if (!(obj instanceof DocumentBatchVisitor)) { + throw new SyntaxError("Document Batch is of unexpected type"); Review Comment: ```suggestion throw new SyntaxError(DOCUMENT_BATCH_KEY + " is absent or of unexpected type: " + obj); ``` -- 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
