Add client_session_keep_alive parameter for Snowflake connection

[borisuvarov]

This is a way to mitigate the issue #827 (prevent session timeout after 4 hours of inactivity).

Hey @drewbanin, could you please take a look if you get a chance? Thanks in advance!

[drewbanin]

Thanks for the PR @borisuvarov! This is a really clever implementation! I'm probably not the right person to review this anymore, but someone from Fishtown will take a look :)

[borisuvarov]

Looks like there is a permission issue when running an AppVeyor build https://ci.appveyor.com/project/DrewBanin/dbt/build/1.0.1426-development:

dbt: DEBUG: Snowflake error: 002003 (02000): 451371ad-8f4f-4024-b69d-80335d49e945: SQL compilation error:
Schema 'APPVEYOR_TEST.TEST15360498161559_SIMPLE_COPY_001' does not exist.
dbt: DEBUG: On seed: ROLLBACK
dbt: INFO: 08:36:17 | 1 of 1 ERROR loading seed file test15360498161559_simple_copy_001.seed [ERROR in 4.27s]

Could you please clarify if should it be fixed on your side, or there is a problem in my PR? Thanks!

[drewbanin]

I think that CI failure has to do with the deletion of this line: https://github.com/fishtown-analytics/dbt/pull/970/files#diff-12e0c6e7584ec33b980b924983dab7eeL133

[drewbanin]

It could also be related to this change: https://github.com/fishtown-analytics/dbt/pull/970/files#diff-12e0c6e7584ec33b980b924983dab7eeL21

[borisuvarov]

@drewbanin Thanks a lot, and sorry about this. My bad! Fixed, let's see if it helps.

[borisuvarov]

@cmcarthur @drewbanin Hey guys, could you please take a quick look if you get a chance? It would be great to know if this PR is acceptable, or should I adjust/improve it somehow. Sorry for bugging you, but we're still suffering from this timeout issue. Thanks in advance!

[drewbanin]

hey - thanks for the ping @borisuvarov! @beckjake and I spoke about this the other day and then we lost track of it!

I think this is a really clever approach, but I worry that it might be too clever. It's difficult to manage threads in Python when errors occur, and a hanging thread can prevent dbt from exiting. So, if we need to do this, I'm into it, but I'd prefer an approach that doesn't require threading if possible.

Did you already evaluate setting CLIENT_SESSION_KEEP_ALIVE=false as described in #827? That configuration would disable the timeout entirely, so there wouldn't be any need for a heartbeat at all! This is a bit of a heavy-handed approach, but I think it should be acceptable for most uses of dbt and vastly simpler to implement and maintain.

I'm digging in to find out how to set CLIENT_SESSION_KEEP_ALIVE using snowflake-connector-python. I've only been looking for a couple of minutes, but haven't been able to find it yet. I'm very happy to keep looking though!

I am very happy to discuss - let me know what you think! And thanks again for following up :)

[borisuvarov]

Hey Drew, I totally understand your concerns about threads, but looks like CLIENT_SESSION_KEEP_ALIVE is available only in ODBC and JDBC clients, but not in their Python connector: https://docs.snowflake.net/manuals/sql-reference/parameters.html#client-session-keep-alive

I'll try to reach out, maybe I'll manage to add that parameter into the Python Snowflake connector. I'll get back to you with an answer from Snowflake guys.

Thanks!

[drewbanin]

ah!

Currently, the parameter cannot be set at the session level by executing the ALTER SESSION command. For information on setting the parameter at the session level, see the JDBC or ODBC documentation.

That's annoying -- i figured we could just run a query to set that on every connection, but looks like that won't work either.

I think reaching out is definitely the move! Let me know how it goes!

[borisuvarov]

Please see the discussion here: snowflakedb/snowflake-connector-python#104
Looks like I should refactor this PR in order to leverage CLIENT_SESSION_KEEP_ALIVE of the native connector, which will be added soon.

[borisuvarov]

Thanks to https://github.com/smtakeda, the CLIENT_SESSION_KEEP_ALIVE is going to be included into the upcoming 1.6.10 release of the Snowflake Python connector: snowflakedb/snowflake-connector-python@2ad0269#diff-9bafad465c7c8135c979e1e45c1e9102R14 I'll update this PR tomorrow.

[drewbanin]

Thanks @borisuvarov - this is exciting! Let's be sure to close our connections:

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.

This would prevent dbt from exiting, I think. I'd also like to bypass this code entirely if possible, as users who don't set a keepalive shouldn't be impacted by any threading issues like this.

Looking forward to the updated PR!

[borisuvarov]

Hey @drewbanin, could you please take a look? Actually now it's just one line. :-) Thanks!

[beckjake]

Hi @borisuvarov, this looks mostly great to me, I had one review comment.

Also, could you please rebase this against the branch named dev/guion-bluford and change the PR to be against that? It's our current 'dev' branch that will most likely become our next release.

[beckjake]

I think this should have a , False so the default is False, not None.

[borisuvarov]

Hey @beckjake, thanks for your review! I've fixed this PR accordingly to your comments, please have a look when you get a chance.

[beckjake]

Looks great, thanks! I've triggered the non-windows tests for completeness, once those pass (and I assume they will) this will be ready to merge.