Fix grpc upload retry timeout #841
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
✅ Deploy Preview for poetic-froyo-8baba7 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
hh-space-invader
force-pushed
the
fix-grpc-upload-retry-timeout
branch
from
a147de6 to
25d452d
Compare
new: Add timeouts to delete and upsert
new: Added missing timeouts
hh-space-invader
force-pushed
the
fix-grpc-upload-retry-timeout
branch
from
2dfe1a4 to
bc85c04
Compare
joein
requested changes
Nov 14, 2024
qdrant_client/connection.py
Outdated
| @@ -200,9 +201,23 @@ async def intercept_call( | |||
|
|
|||
|
|
|||
| def parse_channel_options(options: Optional[Dict[str, Any]] = None) -> List[Tuple[str, Any]]: | |||
| timeout = f'{str(options.copy().pop("timeout", "10"))}s' if options else "10s" | |||
There was a problem hiding this comment.
do we need to extract timeout from options anywhere else?
| @@ -2912,6 +2916,7 @@ def _upload_collection( | |||
| "metadata": self._grpc_headers, | |||
| "wait": wait, | |||
| "shard_key_selector": shard_key_selector, | |||
| "options": self._grpc_options, | |||
There was a problem hiding this comment.
i think it's enough to pass just timeout rather than the whole options
There was a problem hiding this comment.
I was wrong, it's actually a bug, we've been needing to pass it all the time
qdrant_client/connection.py
Outdated
Comment on lines
208
to
220
| ( | ||
| "grpc.service_config", | ||
| json.dumps( | ||
| { | ||
| "methodConfig": [ | ||
| { | ||
| "name": [{}], | ||
| "timeout": timeout, | ||
| }, | ||
| ] | ||
| } | ||
| ), | ||
| ), |
There was a problem hiding this comment.
it feels to be too much for this, we already have timeouts in all the methods
- it is a little bit ugly because we need to create a json string
- if anyone is providing or will provide
grpc.service_configviagrpc_optionsit will break things - it does not work as you assume when you look at it, I've done several experiments and it has never waited for
timeout, but fortimeout - 1sortimeout - 2s. Thus, if we set timeout=1 it just raises an Exception immediately
I would rather add GRPC_DEFAULT_TIMEOUT or something like that to QdrantRemote
joein
reviewed
Nov 14, 2024
joein
previously approved these changes
Nov 15, 2024
joein
approved these changes
Nov 15, 2024
joein
added a commit
that referenced
this pull request
Jan 16, 2025
* improve: Added a default timeout in grpc if the connection is dead * improve: Added 5 seconds wait before retry * fix: Fix grpc timeout * nit * fix: Fix bug in timeouts new: Add timeouts to delete and upsert * new: Propagate timeout to grpc default options new: Added missing timeouts * nit * refactor: Changed how we define the default grpc timeout * remove redundant import * fix: add missing timeouts * fix: Added missing timeout in rest * fix: Added async --------- Co-authored-by: George <george.panchuk@qdrant.tech>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Rest has a default timeout (mostly managed by the network stack of the OS if the requests library default to none), but grpc doesn't have a default one. I've added a default value in the channel options.
There's also no default waiting mechanism for waiting before retrying.
All Submissions:
devbranch. Did you create your branch fromdev?New Feature Submissions:
pre-commitwithpip3 install pre-commitand set up hooks withpre-commit install?Changes to Core Features: