[
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: [email protected]
For additional commands, e-mail: [email protected]
