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.

Sign up for GitHub

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

Jump to bottom

Fix redundant retries for non-multi-tenancy collections when querying tenants #6458

Merged
merged 2 commits into from
Nov 28, 2024
Merged

Fix redundant retries for non-multi-tenancy collections when querying tenants #6458

merged 2 commits into from
Nov 28, 2024

Conversation

redouan-rhazouani
Copy link
Contributor

This PR resolves issue where requests to query tenants for collections that do not use multi-tenancy result in prolonged response times and unnecessary retries

Problem

  1. When a serving node receives a request to query tenants for a specific class, it forwards the request to the leader node via an RPC call.
  2. The leader node unnecessarily retries querying the schema even when it receives an error indicating that multi-tenancy is not enabled for the class.
  3. If a non-leader node also retries the request to the leader, this further prolong the waiting time.

What's being changed:

  • Restrict retries to retryable sections: Only retry operations that are genuinely retryable, such as leader identification.
  • Stop retries for non-supported classes: Prevent non-leader nodes from retrying requests when it is known that the class does not support multi-tenancy.

Review checklist

  • Documentation has been updated, if necessary. Link to changed documentation:
  • Chaos pipeline run or not necessary. Link to pipeline:
  • All new code is covered by tests where it is reasonable.
  • Performance tests have been run or not necessary.

orca-security-eu[bot]
orca-security-eu bot reviewed Nov 25, 2024
View reviewed changes
Copy link

@orca-security-eu orca-security-eu bot left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Orca Security Scan Summary

Status Check Issues by priority
Passed Passed Infrastructure as Code high 0   medium 0   low 0   info 0 View in Orca
Passed Passed Secrets high 0   medium 0   low 0   info 0 View in Orca
Passed Passed Vulnerabilities high 0   medium 0   low 0   info 0 View in Orca

@redouan-rhazouani redouan-rhazouani mentioned this pull request Nov 25, 2024
Closed
1 task
@redouan-rhazouani
Fix: avoid redundant rpc retries when querying tenants from non-multi…
92ace2a 
…-tenancy collections

 - Ensure only retrybale code section are retried
 - Prevent non-leader nodes from retrying getting tenants when the class does not support multi-tenancy
 - Improve response times by eliminating unnecessary retries
This resolve the issue weaviate#6261 of prolonged response times when querying tenants
@weaviate-git-bot
Copy link

Great to see you again! Thanks for the contribution.

beep boop - the Weaviate bot 👋🤖

PS:
Are you already a member of the Weaviate Slack channel?

@redouan-rhazouani
Copy link
Contributor Author

Hi @moogacs @nathanwilk7 @reyreaud-l ,

Could you please take a moment to review this PR? It's a small change and I'm happy to address any issues if there are any.

Thanks in advance!

Red

@moogacs moogacs added the community originated from the community label Nov 27, 2024
@moogacs
Copy link
Contributor

moogacs commented Nov 27, 2024

@redouan-rhazouani thanks for the contribution 🙏🏼 , we will look at it soon

moogacs
moogacs reviewed Nov 28, 2024
View reviewed changes
Copy link
Contributor

@moogacs moogacs left a comment

Choose a reason for hiding this comment

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

Looks good, left 1 suggestion for readability

cluster/raft_query_endpoints.go Outdated Show resolved Hide resolved
@reyreaud-l
Copy link
Contributor

Hey @redouan-rhazouani thank you for contributing! We verified and we need to you explicitly approve the CLA agreement in order to accept the PR. Thanks in advance :)

@redouan-rhazouani
Copy link
Contributor Author

Hey @redouan-rhazouani thank you for contributing! We verified and we need to you explicitly approve the CLA agreement in order to accept the PR. Thanks in advance :)

I explicitly approved the CLA agreement

moogacs
moogacs approved these changes Nov 28, 2024
View reviewed changes
Copy link
Contributor

@moogacs moogacs left a comment

Choose a reason for hiding this comment

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

LGTM, thanks again @redouan-rhazouani 🙏🏼

@moogacs moogacs merged commit 42eb166 into weaviate:main Nov 28, 2024
45 checks passed
@moogacs moogacs mentioned this pull request Nov 28, 2024
Merged
4 tasks
@redouan-rhazouani
Copy link
Contributor Author

It's my pleasure, @moogacs 🙂 Always happy to contribute again as community member! 🚀

@BrewTestBot BrewTestBot mentioned this pull request Dec 11, 2024
Merged
ajit283 pushed a commit that referenced this pull request Dec 27, 2024
@moogacs
Merge pull request #6458 from redouan-rhazouani/issue_6261_query_tenants
f22facd 
Fix redundant retries for non-multi-tenancy collections when querying tenants
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@orca-security-eu orca-security-eu[bot] orca-security-eu[bot] left review comments

@moogacs moogacs moogacs approved these changes

@reyreaud-l reyreaud-l Awaiting requested review from reyreaud-l

Assignees
No one assigned
Labels
community originated from the community
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

"Get tenants" is retried for 20s on non-MT class
4 participants
@redouan-rhazouani @weaviate-git-bot @moogacs @reyreaud-l