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-723750: Panic with readonly file system #828

Merged
merged 9 commits into from
Jun 23, 2023

Conversation

sfc-gh-pbulawa
Copy link
Collaborator

Description

Fixed driver panicking when the temp filesystem is readonly.

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

file_transfer_agent.go Outdated Show resolved Hide resolved
file_transfer_agent.go Show resolved Hide resolved
file_transfer_agent_test.go Show resolved Hide resolved
file_transfer_agent_test.go Outdated Show resolved Hide resolved
@mihaitodor
Copy link

Thank you for starting this work! Will it be possible to disable the temp file creation mechanism completely for certain workflows? It's not fully clear to me when it's beneficial, especially when the data is streamed from/to a buffer or a file. Also, when one wishes to run a quick INSERT statement with little data, it would be nice to not have to write it to disk first.

@sfc-gh-dszmolka mentioned in this comment that encryption/decription also creates temp files. Will this PR also address that workflow?

Also, it might be worth considering a Snowflake-specific name for the env var which controls the location of this temporary directory, maybe something like SNOWFLAKE_TMPDIR. People might want to have separate directories for various frameworks and relying solely on TMPDIR makes it difficult to distinguish what came from where.

@sfc-gh-pbulawa
Copy link
Collaborator Author

sfc-gh-pbulawa commented Jun 21, 2023

Thank you for starting this work! Will it be possible to disable the temp file creation mechanism completely for certain workflows? It's not fully clear to me when it's beneficial, especially when the data is streamed from/to a buffer or a file. Also, when one wishes to run a quick INSERT statement with little data, it would be nice to not have to write it to disk first.

Removing the usage of the temp folder is not planned.

@sfc-gh-dszmolka mentioned in #700 (comment) that encryption/decription also creates temp files. Will this PR also address that workflow?

The encryption/decryption workflow was not affected with this bug - the creation of the temp file itself does not panic but returns the error which was not properly handled. The driver wanted to assign the error to the nil result which panicked.

Also, it might be worth considering a Snowflake-specific name for the env var which controls the location of this temporary directory, maybe something like SNOWFLAKE_TMPDIR. People might want to have separate directories for various frameworks and relying solely on TMPDIR makes it difficult to distinguish what came from where.

That is a very good idea - we have prepared a feature request to expand the driver with such functionality.

@sfc-gh-pbulawa sfc-gh-pbulawa force-pushed the SNOW-723750-panic-with-readonly-filesystem branch from bcae783 to 65e5b57 Compare June 21, 2023 15:28
file_transfer_agent.go Outdated Show resolved Hide resolved
file_transfer_agent_test.go Outdated Show resolved Hide resolved
@sfc-gh-pbulawa sfc-gh-pbulawa force-pushed the SNOW-723750-panic-with-readonly-filesystem branch from ae5f8ec to 2dfbeb7 Compare June 22, 2023 13:00
Copy link
Collaborator

@sfc-gh-igarish sfc-gh-igarish left a comment

Choose a reason for hiding this comment

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

LGTM

@sfc-gh-pbulawa sfc-gh-pbulawa force-pushed the SNOW-723750-panic-with-readonly-filesystem branch from 8ea4fb3 to 35746df Compare June 23, 2023 07:03
@sfc-gh-pbulawa sfc-gh-pbulawa merged commit a832856 into master Jun 23, 2023
@sfc-gh-pbulawa sfc-gh-pbulawa deleted the SNOW-723750-panic-with-readonly-filesystem branch June 23, 2023 07:48
@github-actions github-actions bot locked and limited conversation to collaborators Jun 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants