[jira] [Comment Edited] (SOLR-17926) Discount timeAllowed for all types of queries

["Chris M. Hostetter (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/SOLR-17926?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18022857#comment-18022857 ] 

Chris M. Hostetter edited comment on SOLR-17926 at 9/25/25 5:03 PM:
--------------------------------------------------------------------

{quote}... so it's a bug in the PR, the param should be retrieved _*also*_ in the parent req, outside the conditional.
{quote}
Hmmm.. why "also" ? ... -shouldn't- couldn't the value be used exclusively in the parent?

In the latest PR you do the calculation redundantly in both the parent and child...
 * TimeAllowedLimit constructor always sets {{reqInflightMs = req.getParams().getLong(INFLIGHT_PARAM, DEFAULT_INFLIGHT_MS);}}
 * In the parent code path:
 ** adjustShardRequestLimit skips the child request if: {{usedTimeAllowedMs >= reqTimeAllowedMs - reqInflightMs}}
 * In the child code path:
 ** constructor adjusts based on both values: {{timeAlreadySpentMs += parentUsedMs + reqInflightMs}}

Why not instead just do the calculation once in the parent and use the same value for the conditional "should i send this shard request?" logic as well as the value of the {{USED_PARAM}} ?
 * have the parent include {{reqInflightMs}} when adding up the  {{usedTimeAllowedMs}}
 * then the parent's call to {{{}adjustShardRequestLimit{}}}:
 ** skip the child request if: {{usedTimeAllowedMs >= reqTimeAllowedMs}}
 ** {{params.set(USED_PARAM, Long.toString(usedTimeAllowedMs));}}
 * And the child code path in the constructor can trivially just: {{timeAlreadySpentMs += parentUsedMs}}

?


was (Author: hossman):
{quote}... so it's a bug in the PR, the param should be retrieved _*also*_ in the parent req, outside the conditional.
{quote}
Hmmm.. why "also" ? ... shouldn't the value be used exclusively in the parent?

In the latest PR you do the calculation redundantly in both the parent and child...
 * TimeAllowedLimit constructor always sets {{reqInflightMs = req.getParams().getLong(INFLIGHT_PARAM, DEFAULT_INFLIGHT_MS);}}
 * In the parent code path:
 ** adjustShardRequestLimit skips the child request if: {{usedTimeAllowedMs >= reqTimeAllowedMs - reqInflightMs}}
 * In the child code path:
 ** constructor adjusts based on both values: {{timeAlreadySpentMs += parentUsedMs + reqInflightMs}}

Why not instead just do the calculation once in the parent and use the same value for the conditional "should i send this shard request?" logic as well as the value of the {{USED_PARAM}} ?
 * have the parent include {{reqInflightMs}} when adding up the  {{usedTimeAllowedMs}}
 * then the parent's call to {{{}adjustShardRequestLimit{}}}:
 ** skip the child request if: {{usedTimeAllowedMs >= reqTimeAllowedMs}}
 ** {{params.set(USED_PARAM, Long.toString(usedTimeAllowedMs));}}
 * And the child code path in the constructor can trivially just: {{timeAlreadySpentMs += parentUsedMs}}

?

> Discount timeAllowed for all types of queries
> ---------------------------------------------
>
>                 Key: SOLR-17926
>                 URL: https://issues.apache.org/jira/browse/SOLR-17926
>             Project: Solr
>          Issue Type: Improvement
>    Affects Versions: 9.9
>            Reporter: Andrzej Bialecki
>            Assignee: Andrzej Bialecki
>            Priority: Major
>              Labels: pull-request-available
>         Attachments: SOLR-17926-using-NOW.patch
>
>          Time Spent: 10m
>  Remaining Estimate: 0h
>
> Spin-off from SOLR-17869.
> Currently only {{TopGroupsShardRequestFactory}} subtracts the time already spent on local request processing from {{timeAllowed}} before sending shard requests.
> This is inconsistent and likely not optimal. Since {{timeAllowed}} tracks wall-clock time it makes sense to track the same starting point for all phases of distributed request processing and terminate processing early when the allowed time runs out, as compared to the original starting point.
> This is not the way it works now, though (except for this special case of grouping queries): the same time span is allocated to the query coordinator and to the shard requests where the processing starts later, which means that the coordinator may time out while waiting for responses even if all shard requests succeeded.
> [~dsmiley] suggested to use {{SolrRequestInfo.getNOW()}} instead, as the absolute starting point for both local and distributed requests, and compare {{timeAllowed}} to that starting point. However, this relies on correct time sync between nodes.



--
This message was sent by Atlassian Jira
(v8.20.10#820010)

---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org