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

Cloud Fetch download handler #127

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.

Signed-off-by: Matthew Kim <11141331+mattdeekay@users.noreply.github.com>
@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
…frame markers

Signed-off-by: Matthew Kim <11141331+mattdeekay@users.noreply.github.com>
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?

src/databricks/sql/cloudfetch/downloader.py Outdated Show resolved Hide resolved
src/databricks/sql/cloudfetch/downloader.py Outdated Show resolved Hide resolved
src/databricks/sql/cloudfetch/downloader.py Outdated Show resolved Hide resolved
src/databricks/sql/cloudfetch/downloader.py Outdated Show resolved Hide resolved
src/databricks/sql/cloudfetch/downloader.py Outdated Show resolved Hide resolved
src/databricks/sql/cloudfetch/downloader.py Outdated Show resolved Hide resolved
src/databricks/sql/cloudfetch/downloader.py Outdated Show resolved Hide resolved
src/databricks/sql/cloudfetch/downloader.py Outdated Show resolved Hide resolved
src/databricks/sql/cloudfetch/downloader.py Outdated Show resolved Hide resolved
src/databricks/sql/cloudfetch/downloader.py Outdated Show resolved Hide resolved
 - 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 <11141331+mattdeekay@users.noreply.github.com>
@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

Signed-off-by: Matthew Kim <11141331+mattdeekay@users.noreply.github.com>
src/databricks/sql/cloudfetch/downloader.py Outdated Show resolved Hide resolved
Signed-off-by: Matthew Kim <11141331+mattdeekay@users.noreply.github.com>
Signed-off-by: Matthew Kim <11141331+mattdeekay@users.noreply.github.com>
@mattdeekay mattdeekay merged commit 64be9bc into databricks:main Jun 27, 2023
14 checks passed
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 <11141331+mattdeekay@users.noreply.github.com>

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

Signed-off-by: Matthew Kim <11141331+mattdeekay@users.noreply.github.com>

* 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 <11141331+mattdeekay@users.noreply.github.com>

* Changing logger.debug to remove url

Signed-off-by: Matthew Kim <11141331+mattdeekay@users.noreply.github.com>

* _reset() comment to docstring

Signed-off-by: Matthew Kim <11141331+mattdeekay@users.noreply.github.com>

* link_expiry_buffer -> link_expiry_buffer_secs

Signed-off-by: Matthew Kim <11141331+mattdeekay@users.noreply.github.com>

---------

Signed-off-by: Matthew Kim <11141331+mattdeekay@users.noreply.github.com>
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.

None yet

3 participants