cpoerschke commented on a change in pull request #1486:
URL: https://github.com/apache/lucene-solr/pull/1486#discussion_r420621435
##########
File path: solr/core/src/java/org/apache/solr/handler/GraphHandler.java
##########
@@ -83,6 +84,7 @@
private StreamFactory streamFactory = new DefaultStreamFactory();
private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
private String coreName;
+ private SolrClientCache solrClientCache;
Review comment:
minor: here the ordering is coreName/solrClientCache but in the inform
method the use order is solrClientCache/coreName. no particular order seems to
be necessary technically speaking and so it might be helpful in terms of code
readability to use the same ordering (one or the other) in both places?
##########
File path: solr/core/src/java/org/apache/solr/handler/sql/CalciteSolrDriver.java
##########
@@ -59,11 +81,346 @@ public Connection connect(String url, Properties info)
throws SQLException {
if(schemaName == null) {
throw new SQLException("zk must be set");
}
- rootSchema.add(schemaName, new SolrSchema(info));
+ SolrSchema solrSchema = new SolrSchema(info);
+ rootSchema.add(schemaName, solrSchema);
// Set the default schema
calciteConnection.setSchema(schemaName);
- return connection;
+ return new SolrCalciteConnectionWrapper(calciteConnection, solrSchema);
+ }
+
+ // the sole purpose of this class is to be able to invoke SolrSchema.close()
+ // when the connection is closed.
+ private static final class SolrCalciteConnectionWrapper implements
CalciteConnection {
Review comment:
> ... a new top level class that is a no-frills Wrapper ...
You mean like a `FilterCalciteConnection` assuming `Filter...` is still the
naming convention e.g. as in
https://github.com/apache/lucene-solr/blob/releases/lucene-solr/8.5.1/lucene/core/src/java/org/apache/lucene/index/FilterCodecReader.java
say?
I was going to suggest w.r.t. test coverage to ensure all that needs to be
overridden has been overridden (both now and in future) -- similar to
https://github.com/apache/lucene-solr/blob/releases/lucene-solr/8.5.1/lucene/core/src/test/org/apache/lucene/index/TestFilterCodecReader.java#L29
-- and yeah if the no-frills wrapper has the test coverage then it would be
easier to see how `SolrCalciteConnectionWrapper extends
FilterCalciteConnection` is only different w.r.t. the close() method.
##########
File path:
solr/core/src/java/org/apache/solr/metrics/reporters/solr/SolrReporter.java
##########
@@ -306,11 +312,11 @@ public String toString() {
// We delegate to registries anyway, so having a dummy registry is harmless.
private static final MetricRegistry dummyRegistry = new MetricRegistry();
- public SolrReporter(HttpClient httpClient, Supplier<String> urlProvider,
SolrMetricManager metricManager,
+ public SolrReporter(SolrClientCache solrClientCache, Supplier<String>
urlProvider, SolrMetricManager metricManager,
List<Report> metrics, String handler,
String reporterId, TimeUnit rateUnit, TimeUnit
durationUnit,
SolrParams params, boolean skipHistograms, boolean
skipAggregateValues,
- boolean cloudClient, boolean compact) {
+ boolean cloudClient, boolean compact, boolean
closeClientCache) {
Review comment:
minor/subjective: `SolrClientCache solrClientCache` and `boolean
closeClientCache` arguments being adjacent might help with readability in the
builder caller, though of course keeping all the `boolean` arguments together
has advantages too
##########
File path: solr/core/src/java/org/apache/solr/search/join/XCJFQuery.java
##########
@@ -261,7 +261,7 @@ private TupleStream createSolrStream() {
}
private DocSet getDocSet() throws IOException {
- SolrClientCache solrClientCache = new SolrClientCache();
+ SolrClientCache solrClientCache =
searcher.getCore().getCoreContainer().getSolrClientCache();
Review comment:
Need the `solrClientCache.close();` further down in the method be
removed since a shared cache is now used?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
