Skip to content

Cloud Fetch download handler #127

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

Merged
merged 6 commits into from
Jun 27, 2023

Conversation

mattdeekay
Copy link
Contributor

CloudFetch download handler. Each handler will run on a thread and a CloudFetch download manager will maintain a thread pool.

@mattdeekay mattdeekay self-assigned this May 16, 2023
@mattdeekay mattdeekay marked this pull request as draft May 16, 2023 21:44
@mattdeekay mattdeekay force-pushed the cloudfetch-downloader branch from 60b036d to 884bc0f Compare May 16, 2023 21:45
@mattdeekay mattdeekay force-pushed the cloudfetch-downloader branch from 2ffb2ba to d0e0037 Compare June 14, 2023 17:26
Signed-off-by: Matthew Kim <[email protected]>
@mattdeekay mattdeekay force-pushed the cloudfetch-downloader branch from d0e0037 to d2ffe1e Compare June 14, 2023 17:44
@mattdeekay mattdeekay changed the title ResultSetDownloadHandler Cloud Fetch download handler Jun 14, 2023
@mattdeekay mattdeekay marked this pull request as ready for review June 14, 2023 18:02
@yunbodeng-db yunbodeng-db requested review from rcypher-databricks and removed request for arikfr June 20, 2023 21:45
@mattdeekay mattdeekay changed the base branch from cloudfetch to main June 20, 2023 21:47
Copy link
Contributor

@susodapop susodapop left a comment

Choose a reason for hiding this comment

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

Do we have integration tests for this part? Or are those in a subsequent PR?

 - Linting
 - Type annotations
 - Use response.ok
 - Log exception
 - Remove semaphore and only use threading.event
 - reset() flags method
 - Fix tests after removing semaphore
 - Link expiry logic should be in secs
 - Decompress data static function
 - link_expiry_buffer and static public methods
 - Docstrings and comments

Signed-off-by: Matthew Kim <[email protected]>
@mattdeekay
Copy link
Contributor Author

Do we have integration tests for this part? Or are those in a subsequent PR?

Nope, will be part of the full e2e PR

@mattdeekay mattdeekay force-pushed the cloudfetch-downloader branch from 9872995 to e8d2f77 Compare June 27, 2023 22:17
@mattdeekay mattdeekay merged commit 64be9bc into databricks:main Jun 27, 2023
susodapop pushed a commit to unj1m/databricks-sql-python that referenced this pull request Sep 19, 2023
* Cloud Fetch download handler

Signed-off-by: Matthew Kim <[email protected]>

* Issue fix: final result link compressed data has multiple LZ4 end-of-frame markers

Signed-off-by: Matthew Kim <[email protected]>

* Addressing PR comments
 - Linting
 - Type annotations
 - Use response.ok
 - Log exception
 - Remove semaphore and only use threading.event
 - reset() flags method
 - Fix tests after removing semaphore
 - Link expiry logic should be in secs
 - Decompress data static function
 - link_expiry_buffer and static public methods
 - Docstrings and comments

Signed-off-by: Matthew Kim <[email protected]>

* Changing logger.debug to remove url

Signed-off-by: Matthew Kim <[email protected]>

* _reset() comment to docstring

Signed-off-by: Matthew Kim <[email protected]>

* link_expiry_buffer -> link_expiry_buffer_secs

Signed-off-by: Matthew Kim <[email protected]>

---------

Signed-off-by: Matthew Kim <[email protected]>
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