Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(cts): test the timeouts of the retry stragety #3264

Merged
merged 7 commits into from
Jul 3, 2024
Merged

Conversation

millotp
Copy link
Collaborator

@millotp millotp commented Jun 27, 2024

🧭 What and Why

🎟 JIRA Ticket: APIC-587

Use the test server to verify that the client uses the correct timeouts and delays.

@millotp millotp self-assigned this Jun 27, 2024
@millotp millotp requested a review from a team as a code owner June 27, 2024 10:57
@millotp millotp requested review from morganleroi and shortcuts June 27, 2024 10:57
@algolia algolia deleted a comment from algolia-bot Jul 3, 2024
Copy link

github-actions bot commented Jul 3, 2024

Copy link
Member

@shortcuts shortcuts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is huge

scripts/cts/testServer/timeout.ts Outdated Show resolved Hide resolved
}
break;
default:
// the delay should be the same, because the `retryCount` is per host instead of global
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't the default be exponential?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

idk what's expected but it feels weird to me to retry instantly

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah almost all clients behave like this except for php and js, basically the retryCount is per host so the delay will increase on the next request, not on the next host, so the delay is the same.
If we run a second query in this test, we will see increased delay, but then the test will take too long.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah so:

  1. query search
  2. fails, retried on all hosts with no exponential delay between hosts
  3. query search again
  4. fails, retried with increased delay, still not exponential between hosts

? this seems wrong to me because we assume the user retries but it's the client that should handle all that 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah its weird, but it was like that in legacy clients, this PR aims at checking the logic, not fixing every client as it would be very complicated

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup my comment doens't block it, I'm trying to understand if we should fix that before GA

I think we should ask the search team for advices and harmonize everything

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we have time yes, it shouldn't be a breaking change to modify this logic

@algolia-bot
Copy link
Collaborator

algolia-bot commented Jul 3, 2024

✔️ Code generated!

Name Link
🪓 Triggered by fc3ee11171d5a5355e5bd9088e9cb33401c8d650
🍃 Generated commit 4612911e02ab16f45338259007bf6dde3352b259
🌲 Generated branch generated/feat/timeout-test

@algolia algolia deleted a comment from github-actions bot Jul 3, 2024
@millotp millotp requested a review from shortcuts July 3, 2024 15:12
Copy link
Member

@shortcuts shortcuts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nothing to add, super clean!

@millotp millotp merged commit 868fc9a into main Jul 3, 2024
20 checks passed
@millotp millotp deleted the feat/timeout-test branch July 3, 2024 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants