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

SNOW-209092 Skip Session Close #354

Merged
merged 2 commits into from
Mar 23, 2021
Merged

Conversation

sfc-gh-jbahk
Copy link
Collaborator

@sfc-gh-jbahk sfc-gh-jbahk commented Dec 1, 2020

Description

Added a control flow to keep client session alive if parameter enabled.

Checklist

  • Code compiles correctly
  • Run make fmt to fix inconsistent formats
  • Run make lint to get lint errors and fix all of them
  • Created tests which fail without the change (if possible)
  • All tests passing
  • Extended the README / documentation, if necessary

@sfc-gh-anezvigin
Copy link

My concern with using isClientSessionKeepAliveEnabled() is that if you turn on keep-alive, there's no way to close the session even if you wanted to.

@sfc-gh-anezvigin
Copy link

Also this is a pretty big behavior change for existing users of the driver that are turning off keep alive. Their sessions will stay open on the GS side.

@sfc-gh-jbahk
Copy link
Collaborator Author

I was thinking that users already enable the client_session_keep_alive to have the session persist and turn it off, followed by Close() to properly close the session. What do you think?

@sfc-gh-cshi
Copy link
Contributor

Seems we don't have end to end test for this session persistence change for go driver. Should we have one?

@sfc-gh-anezvigin
Copy link

I was thinking that users already enable the client_session_keep_alive to have the session persist and turn it off, followed by Close() to properly close the session. What do you think?

Problem is that Close() does more than just optionally close the snowflake session. Go's SQL interface uses it to clean up resources (eg: open channels, etc) that it uses to manage the query lifecycle. The user must call Close() when using the driver in long running processes.

So the following scenario would create a resource leak:

  • Long running process (eg: API server)
  • Sets client_session_keep_alive to false
  • Does not call Close() on the sql.DB object

Copy link

@sfc-gh-anezvigin sfc-gh-anezvigin left a comment

Choose a reason for hiding this comment

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

👍

connection.go Outdated
if err != nil {
logger.Error(err)
if !sc.cfg.KeepSessionAlive {
err = sc.rest.FuncCloseSession(context.TODO(), sc.rest, sc.rest.RequestTimeout)
Copy link
Collaborator

Choose a reason for hiding this comment

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

context.Todo() or should this be sc.ctx

Copy link
Collaborator

@sfc-gh-dwu sfc-gh-dwu left a comment

Choose a reason for hiding this comment

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

lgtm with 1 ctx change

@sfc-gh-jbahk sfc-gh-jbahk force-pushed the SNOW-209092-skip-session-close branch from dc57405 to bf76890 Compare March 23, 2021 00:51
@sfc-gh-jbahk sfc-gh-jbahk merged commit d66efdf into master Mar 23, 2021
@sfc-gh-jbahk sfc-gh-jbahk deleted the SNOW-209092-skip-session-close branch March 23, 2021 01:31
GregOwen pushed a commit to sigmacomputing/gosnowflake that referenced this pull request May 21, 2021
* enable session active when calling close()

* add unit test
GregOwen pushed a commit to sigmacomputing/gosnowflake that referenced this pull request May 21, 2021
* enable session active when calling close()

* add unit test
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

4 participants