Hi @varshith257, thank you for your contribution 👋

Introduced the calculate_timeout function to handle precise floating-point timeout values, ensuring timeout clamping to a minimum of 1 second.

I would propose a slightly different approach, the changes should happen only the remote shard operations and not across the code base.

The function now supports both std::time::Instant and tokio::time::Instant for flexibility across codebases.

I do not believe those new abstractions are useful TBH.

Instead I propose to apply two changes for the 6 ops in remote_shards:

• scroll_by
• core_search
• count
• retrieve
• query_batch
• facet

First, start by checking if the duration is zero, in that case we do not need to call the remote shards and can shortcut the requests with a timeout directly to save on network traffic.

Secondly, about handling the flooring issue of timeout_duration.as_secs which turns anything under 1 second to zero.

I would simply replace
timeout.map(|t| t.as_secs())
by
timeout.map(|t| t.as_secs().max(1)

In order to ensure the validation is not triggered on the server side.

WDYT?

Also please test your changes locally before submitting a PR :)

@agourlay I agree. I'd rather keep using the std Duration API everywhere possible. It already has sufficient arithmetic functions.

timeout.map(|t| t.as_secs().max(1)

You suggest lower-capping at at least 1.

How about using t.as_sec_f32().ceil() as usize instead?

I believe these changes partially address the issue:

• When the user specifies a 1-second timeout, it is always rounded up to 1 second, introducing errors in the estimation.
• If the user provides a timeout greater than 1 second, the flooring issue remains. For example, if the user specifies 2 seconds, a “deadline exceeded” error could still be returned after only 1 second.

What about handling timeouts in milliseconds (ms) between nodes?

What about handling timeouts in milliseconds (ms) between nodes?

I'd prefer seconds as floating point because it's an SI base unit.

Either way it'd require breaking API changes in some places. So it's not so trivial to implement.

Sorry, I wasn’t clear! I’m totally fine with using seconds as a floating-point value. My point was more “we cannot floor or ceil to the nearest second; we need greater precision than an int.”

Thanks for reviewing! I am just bit confused so raised Pr with what I understood 😀

Will push changes as requested

I'm actually seeing this issue with 5 second timeouts as well

The issue can happen for other timeout values where the pre-processing steps prior to reaching the remote shard decrease the value to less than 1 second.

I believe the course of action I described is good enough to also tackle the precision concern given that we cannot introduce breaking changes.

We do set the original timeout on the gRPC connection for each remote shard call with

if let Some(timeout) = timeout {
  request.set_timeout(timeout);
}

In that case the connection will be closed which in turn will drop the future object being processed.

@varshith257 Hi! Would you mind to bump your PR to apply the other suggestion. 😄

Basically just update the calculation mentioned here. And update it to something like t.as_sec_f32().ceil() as usize.

I think that'll be good enough.

Just it went messy when rebasing. Sorry for it! I have opened new PR

FYI I have implemented my suggestions in #5495