-
Notifications
You must be signed in to change notification settings - Fork 22
optimize management api calls #110
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
base: main
Are you sure you want to change the base?
Conversation
singlestoredb/management/files.py
Outdated
| *, | ||
| overwrite: bool = False, | ||
| encoding: Optional[str] = None, | ||
| _skip_dir_check: bool = False, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than add this to a publicly visible API, how about we create a _download_file internal method with the extra parameter that is called by download_file. So the parameters for download_file stay the way they were before, but we call the _download_file internal version with _skip_dir_check when needed. That keeps internally used parameters out of the generated documentation.
9a4dd3e to
18a1f4c
Compare
| self, | ||
| path: PathLike = '/', | ||
| *, | ||
| recursive: bool = False, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't you need a return_meta parameter here too?
| *, | ||
| recursive: bool = False, | ||
| ) -> List[str]: | ||
| ) -> Union[List[str], List[Union[str, Dict[str, Any]]]]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to return str or singlestoredb.management.files.FilesObject it already has the de-serializing code in it and can be type checked. Creating a FilesObject doesn't call any more endpoints and is more functional than just a dictionary as well. It might be better to call the above parameter return_objects in this case as well. It's a bit more descriptive.
We are having redundant API calls to management API for the DOWNLOAD MODEL command.
We are making 3 GET calls for every folder/file , so total GET calls = 3 (for parent folder)+ 3*N (2 checks are redundant as they check if the path is a directory or not but that information is already available in the metadata so these calls can be skipped).
Optimization in this branch -
we make 1 GET call per folder/file so if there is 1 parent folder and N files inside it, total calls now will be 1+N
This reduces the number of calls to 1/3rd or 67% reduction while maintaining the same functionality.