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

Implement Heartbeat for python #104

Closed
williammarino opened this issue Aug 8, 2018 · 8 comments
Closed

Implement Heartbeat for python #104

williammarino opened this issue Aug 8, 2018 · 8 comments
Labels
enhancement The issue is a request for improvement or a new feature
Milestone

Comments

@williammarino
Copy link

Can we get a heartbeat implemented here to eliminate the authentication bug?

@smtakeda
Copy link
Contributor

smtakeda commented Aug 9, 2018

This item has been in the backlog, but we didn't have a chance to work on. There is a design question about how to run heartbeat reliably, and in fact, even JDBC, ODBC and Golang driver don't have perfect solution yet.

@smtakeda smtakeda added enhancement The issue is a request for improvement or a new feature help wanted labels Aug 9, 2018
@borisuvarov
Copy link
Contributor

Hey @smtakeda, I don't know if you're familiar with the dbt, but this tool is using the snowflake-connector-python under the hood, and this problem with timeouts is hitting us big time. I've tried to implement a naive solution for heartbeat (https:/fishtown-analytics/dbt/pull/970/files), but dbt's maintainer wants to know if it's possible to avoid using threads, and leverage CLIENT_SESSION_KEEP_ALIVE option instead. As I understand, it's available in ODBC and JDBC connectors only. So can I ask you if there is a plan to add this parameter into snowflake-connector-python? I'd like to help, if required.

Thanks, and sorry for bugging you! Snowflake is awesome, and snowflake-connector-python as well!

@smtakeda
Copy link
Contributor

Thanks for input and I appreciate your effort on adding heartbeat feature to dbt. I didn't know dbt but it appeared the company's analytics team uses it.

Spinning a thread is problematic everywhere in Python. I understand dbt maintainer's concern, but I'm afraid that Python Connector would use a Timer thread as well :) or potentially async (only for Python3) or sched module. If the timer is ok, I'll try soon. Any suggestion is welcome.

@drewbanin
Copy link

Hey all - one of the dbt maintainers here. Thanks for the writeup @borisuvarov and hello @smtakeda :)

We're really big fans of Snowflake! It works super well with dbt's viewpoint of how data transformation should work, and snowflake-connector-python really helps out with the heavy lifting around connections and querying.

AS @borisuvarov noted, I'm a little hesitant to add a threaded heartbeat like this to dbt. We pool connections to the database and have bit bitten by unclosed connections that prevent dbt from exiting before. Granted, these were bugs in our implementation, but threaded connections add so much surface area for bugs to appear!

If dbt only worked with Snowflake, this would be a much more appealing proposition to us. Instead, dbt works with Redshift and BigQuery as well, so the proposed implementation of heartbeats breaks some of our abstractions and adds a real maintenance burden for us.

I think adding a feature like this to snowflake-connector-python would not only help dbt, but any other Python software which interfaces with Snowflake. I'm very happy to help out with implementing this feature or testing it out once it's ready! Please let me know how I can be helpful!

@borisuvarov
Copy link
Contributor

Hello @smtakeda, I've tried to submit an implementation, but I ran into some issues with setting up local environment for the unit tests. Is this doc is still up to date? (If it is, I'll try to figure it out myself, just checking in advance). Also for some reason the Travis builds are failing with this error: The command "openssl aes-256-cbc -k "$super_secret_password" -in parameters.py.enc -out test/parameters.py -d" failed and exited with 1 during . Is it a known issue?

Thanks, and sorry for bugging you :-)

@smtakeda
Copy link
Contributor

@borisuvarov, thanks for PR. Let me look into it in my next spring anyway. (next week or after)
I'm sorry, but openssl is used to decrypt the passwords for the test account, and it works only if the PR comes from the source branch.

@smtakeda smtakeda mentioned this issue Sep 25, 2018
@smtakeda smtakeda added this to the v1.6.10 milestone Sep 25, 2018
@smtakeda
Copy link
Contributor

Merged #116 including more tests for CLIENT_SESSION_KEEP_ALIVE.

Caveat: if CLIENT_SESSION_KEEP_ALIVE is set and the application doesn't close the connection, the connection will hang due to a thread leak.

I attempted to cancel and join the thread in other ways, i.e., in del method, atexit hook, I could not address it.

smtakeda added a commit that referenced this issue Sep 25, 2018
@borisuvarov
Copy link
Contributor

@smtakeda Thank you very much!

sfc-gh-aling pushed a commit that referenced this issue Oct 17, 2022
timostrunk pushed a commit to timostrunk/snowflake-connector-python that referenced this issue Dec 11, 2023
…2fa114

snowflake-connector-python v2.7.7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement The issue is a request for improvement or a new feature
Projects
None yet
Development

No branches or pull requests

4 participants