You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by GitBox <gi...@apache.org> on 2020/05/05 17:17:59 UTC
[GitHub] [lucene-solr] sigram opened a new pull request #1486: SOLR-14423: Use SolrClientCache instance managed by CoreContainer
sigram opened a new pull request #1486:
URL: https://github.com/apache/lucene-solr/pull/1486
See Jira for more details.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] madrob commented on a change in pull request #1486: SOLR-14423: Use SolrClientCache instance managed by CoreContainer
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #1486:
URL: https://github.com/apache/lucene-solr/pull/1486#discussion_r420459575
##########
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:
Can we have a new top level class that is a no-frills Wrapper, and then extend that here overriding just close? I think it is easier for people to see what is different.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] sigram commented on a change in pull request #1486: SOLR-14423: Use SolrClientCache instance managed by CoreContainer
Posted by GitBox <gi...@apache.org>.
sigram commented on a change in pull request #1486:
URL: https://github.com/apache/lucene-solr/pull/1486#discussion_r420758305
##########
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:
I went a step back to keep the old constructor for back-compat - the class is public and its constructor is public too.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] sigram commented on a change in pull request #1486: SOLR-14423: Use SolrClientCache instance managed by CoreContainer
Posted by GitBox <gi...@apache.org>.
sigram commented on a change in pull request #1486:
URL: https://github.com/apache/lucene-solr/pull/1486#discussion_r420760142
##########
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:
Fixed.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] sigram closed pull request #1486: SOLR-14423: Use SolrClientCache instance managed by CoreContainer
Posted by GitBox <gi...@apache.org>.
sigram closed pull request #1486:
URL: https://github.com/apache/lucene-solr/pull/1486
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] sigram commented on a change in pull request #1486: SOLR-14423: Use SolrClientCache instance managed by CoreContainer
Posted by GitBox <gi...@apache.org>.
sigram commented on a change in pull request #1486:
URL: https://github.com/apache/lucene-solr/pull/1486#discussion_r420638269
##########
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:
Sure, good idea.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] cpoerschke commented on a change in pull request #1486: SOLR-14423: Use SolrClientCache instance managed by CoreContainer
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] sigram commented on a change in pull request #1486: SOLR-14423: Use SolrClientCache instance managed by CoreContainer
Posted by GitBox <gi...@apache.org>.
sigram commented on a change in pull request #1486:
URL: https://github.com/apache/lucene-solr/pull/1486#discussion_r420757857
##########
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:
Yes, good catch!
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org