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-1094021: Add Methods to Datasource to support further Connection Settings #1647

Closed
bahoffmann opened this issue Feb 23, 2024 · 8 comments
Assignees
Labels
feature status-fixed_awaiting_release The issue has been fixed, its PR merged, and now awaiting the next release cycle of the connector. status-merged status-triage_done Initial triage done, will be further handled by the driver team

Comments

@bahoffmann
Copy link

What is the current behavior?

Currently you are limited to not using the 'SnowflakeBasicDataSource' if you for example try to connect to Snowflake from behind a Proxy. This is due to a limit in Setters for the connection properties / the missing Getter for the Properties Object to add supported options.

What is the desired behavior?

I would like to extend the Class with more setters and either set a Getter for Properties or (which i think would be worse) make the Properties protected so that you could atleast extend from the 'SnowflakeBasicDataSource' to not rewrite it completly,

@github-actions github-actions bot changed the title Add Methods to Datasource to support further Connection Settings SNOW-1094021: Add Methods to Datasource to support further Connection Settings Feb 23, 2024
@sfc-gh-wfateem sfc-gh-wfateem self-assigned this Feb 27, 2024
@sfc-gh-wfateem
Copy link
Collaborator

sfc-gh-wfateem commented Feb 27, 2024

Hi @bahoffmann,
Thanks for the comments. I'm thinking maybe it would just be easier to have a single setter and getter method for properties. For example:

SnowflakeBasicDataSource dataSource = new SnowflakeBasicDataSource();
Properties props = dataSource.getProperties();
props.put("proxyHost","127.0.0.1");
props.put("proxyPort","8080");
dataSource.set(props);

Would that suffice?

Just FYI, you can always use the JVM arguments to set proxy settings as documented here.

@bahoffmann
Copy link
Author

bahoffmann commented Feb 27, 2024

Hey @sfc-gh-wfateem ,
that would absolutly solve the problem.

I am on my personal taste unsure if a setter would be that good, as you would overwrite a privatly instantiated object here. That is why i would just provide a getter so the user is able to work on the Snowflake provided object and never accidentally overwrite any values they might have set before. That way we can have both, convenience methods and full control over the connection properties.

I highly prefer having methods or even a builder pattern that supports all connection properties, so i don't have to do a deep search as to what properties are supported and how are they called. Maybe we could get the same on the snowflake-ingest-sdk as that only excepts properties in an builder pattern.

Just FYI, you can always use the JVM arguments to set proxy settings as documented here.

That i didn't know thank you, could be used as a workaround in the future!

@sfc-gh-dszmolka sfc-gh-dszmolka added the status-triage_done Initial triage done, will be further handled by the driver team label Apr 27, 2024
@bahoffmann
Copy link
Author

Are there any updates, on when this will be done? If that helps, i could support this with a pull request.

@sfc-gh-dszmolka
Copy link
Contributor

sfc-gh-dszmolka commented Jun 21, 2024

thank you for your interest in this enhancement request ! the team constantly evaluates them and tries to design/implement as resources and priorities allow. with that said, currently there is no timeline attached to this enhancement request.
if that's within your possibilities, you're more than welcome to submit a PR with the implementation and thank you for your contribution in advance !

edit: if you (or any future reader needing an enhancement implemented) is perhaps already a Snowflake customer; please do reach out to your account team if you did not have done so and word the importance of the particular feature to them. This can help prioritize resources and efforts amongst enhancements (bugs always take priority)

@sfc-gh-wfateem
Copy link
Collaborator

PR created #1800

@sfc-gh-wfateem sfc-gh-wfateem added the status-pr_pending_merge A PR is made and is under review label Jul 8, 2024
@sfc-gh-wfateem sfc-gh-wfateem added status-merged status-fixed_awaiting_release The issue has been fixed, its PR merged, and now awaiting the next release cycle of the connector. and removed status-pr_pending_merge A PR is made and is under review labels Jul 16, 2024
@sfc-gh-wfateem
Copy link
Collaborator

@bahoffmann the PR has been merged and will be part of the next JDBC release.

@sfc-gh-wfateem
Copy link
Collaborator

Hey @bahoffmann,

This has now been released in v3.18.0.

I'll go ahead and close the issue. Thanks for your patience!

@bahoffmann
Copy link
Author

Thank you very much, i will update my projects asap!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature status-fixed_awaiting_release The issue has been fixed, its PR merged, and now awaiting the next release cycle of the connector. status-merged status-triage_done Initial triage done, will be further handled by the driver team
Projects
None yet
Development

No branches or pull requests

3 participants