You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2021/04/15 11:48:09 UTC
[GitHub] [solr] janhoy opened a new pull request #78: Solr15252 log high rows
janhoy opened a new pull request #78:
URL: https://github.com/apache/solr/pull/78
See https://issues.apache.org/jira/browse/SOLR-15252
Please consider only changes in QueryComponent.java, as the other changes is to get Solr compile (#77)
--
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@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a change in pull request #78: SOLR-15252 Solr should log WARN log when a query requests huge rows number
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #78:
URL: https://github.com/apache/solr/pull/78#discussion_r614272064
##########
File path: solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java
##########
@@ -236,6 +240,21 @@ public void prepare(ResponseBuilder rb) throws IOException
if (rb.getSortSpec().getOffset() < 0) {
throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "'start' parameter cannot be negative");
}
+ if(rb.getSortSpec().getCount() > ROWS_WARN_THRESHOLD && shouldLogPeriodically("rowsWarn", 60)) {
+ log.warn("Very high 'rows' parameter detected. This may lead to performance- and memory problems. " +
+ "Consider pagination, see https://solr.apache.org/guide/pagination-of-results.html. " +
+ "This warning will mute for 60s.");
Review comment:
Should the default mute be 3600s instead? 24 WARN logs per day should be enough to draw attention to this issue in most systems.
--
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@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a change in pull request #78: SOLR-15252 Solr should log WARN log when a query requests huge rows number
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #78:
URL: https://github.com/apache/solr/pull/78#discussion_r614138208
##########
File path: solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java
##########
@@ -236,6 +240,21 @@ public void prepare(ResponseBuilder rb) throws IOException
if (rb.getSortSpec().getOffset() < 0) {
throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "'start' parameter cannot be negative");
}
+ if(rb.getSortSpec().getCount() > ROWS_WARN_THRESHOLD && shouldLogPeriodically("rowsWarn", 60)) {
+ log.warn("Very high 'rows' parameter detected. This may lead to performance- and memory problems. " +
+ "Consider pagination, see https://solr.apache.org/guide/pagination-of-results.html. " +
+ "This warning will mute for 60s.");
+ }
+ }
+
+ // Decides whether to log again based on key and interval
+ private static boolean shouldLogPeriodically(String key, int intervalSeconds) {
Review comment:
I know this is a bit of an overkill for just one use. Are you aware of similar logic elsewhere that we could use instead? Or some Util class where this static method could live?
--
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@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] madrob commented on a change in pull request #78: SOLR-15252 Solr should log WARN log when a query requests huge rows number
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #78:
URL: https://github.com/apache/solr/pull/78#discussion_r616059443
##########
File path: solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java
##########
@@ -236,6 +240,21 @@ public void prepare(ResponseBuilder rb) throws IOException
if (rb.getSortSpec().getOffset() < 0) {
throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "'start' parameter cannot be negative");
}
+ if(rb.getSortSpec().getCount() > ROWS_WARN_THRESHOLD && shouldLogPeriodically("rowsWarn", 60)) {
+ log.warn("Very high 'rows' parameter detected. This may lead to performance- and memory problems. " +
+ "Consider pagination, see https://solr.apache.org/guide/pagination-of-results.html. " +
+ "This warning will mute for 60s.");
Review comment:
I think maybe 300s is a better compromise. I'd probably miss something that logs hourly, based on my experiences with heavily used clusters - there are plenty of WARN messages already about slow queries, leader elections, other "normal" operations in a large cluster.
--
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@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy closed pull request #78: SOLR-15252 Solr should log WARN log when a query requests huge rows number
Posted by GitBox <gi...@apache.org>.
janhoy closed pull request #78:
URL: https://github.com/apache/solr/pull/78
--
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@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] cpoerschke commented on a change in pull request #78: SOLR-15252 Solr should log WARN log when a query requests huge rows number
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on a change in pull request #78:
URL: https://github.com/apache/solr/pull/78#discussion_r618591504
##########
File path: solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java
##########
@@ -236,6 +240,21 @@ public void prepare(ResponseBuilder rb) throws IOException
if (rb.getSortSpec().getOffset() < 0) {
throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "'start' parameter cannot be negative");
}
+ if(rb.getSortSpec().getCount() > ROWS_WARN_THRESHOLD && shouldLogPeriodically("rowsWarn", 60)) {
+ log.warn("Very high 'rows' parameter detected. This may lead to performance- and memory problems. " +
+ "Consider pagination, see https://solr.apache.org/guide/pagination-of-results.html. " +
+ "This warning will mute for 60s.");
Review comment:
How about making the logging interval (and the threshold) configurable? I've put up a possible approach in https://github.com/cominvent/solr/compare/solr15252-log-high-rows...cpoerschke:solr15252-log-high-rows and the idea would be that both could be made default (or invariant) parameters on the request handler. Admittedly `rows.warnThresholdLogIntervalSeconds` is a bit of a long name though.
--
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@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] cpoerschke commented on pull request #78: SOLR-15252 Solr should log WARN log when a query requests huge rows number
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on pull request #78:
URL: https://github.com/apache/solr/pull/78#issuecomment-821260166
Any thoughts on also mentioning this in the Solr Ref guide documentation e.g. https://github.com/apache/solr/blob/main/solr/solr-ref-guide/src/common-query-parameters.adoc#rows-parameter or maybe it's hopefully too uncommon to mention in the common parameters context?
--
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@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] cpoerschke commented on pull request #78: SOLR-15252 Solr should log WARN log when a query requests huge rows number
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on pull request #78:
URL: https://github.com/apache/solr/pull/78#issuecomment-821260428
Any thoughts on also mentioning this in the Solr Ref guide documentation e.g. https://github.com/apache/solr/blob/main/solr/solr-ref-guide/src/common-query-parameters.adoc#rows-parameter or maybe it's hopefully too uncommon to mention in the common parameters context?
--
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@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] cpoerschke removed a comment on pull request #78: SOLR-15252 Solr should log WARN log when a query requests huge rows number
Posted by GitBox <gi...@apache.org>.
cpoerschke removed a comment on pull request #78:
URL: https://github.com/apache/solr/pull/78#issuecomment-821260428
Any thoughts on also mentioning this in the Solr Ref guide documentation e.g. https://github.com/apache/solr/blob/main/solr/solr-ref-guide/src/common-query-parameters.adoc#rows-parameter or maybe it's hopefully too uncommon to mention in the common parameters context?
--
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@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org