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.

Sign up for GitHub

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

Jump to bottom

Remove cloud storage SDKs and use the REST API #543

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Remove cloud storage SDKs and use the REST API #543

wants to merge 10 commits into from
+1,191 −675

Conversation

SimbaGithub
Copy link
Collaborator

Based off SF# 00396900

Remove the cloud storage SDKs of AWS, Azure, and GCS and use their REST API for upload/download

@codecov-commenter
Copy link

Codecov Report

Merging #543 (3a26a35) into master (2921db3) will not change coverage.
The diff coverage is n/a.

❗ Current head 3a26a35 differs from pull request most recent head 8fdca7e. Consider uploading reports for the commit 8fdca7e to get more accurate results

@@      Coverage Diff      @@
##   master   #543   +/-   ##
=============================
=============================

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@SimbaGithub SimbaGithub marked this pull request as ready for review November 7, 2022 17:56
sfc-gh-igarish
sfc-gh-igarish reviewed Nov 7, 2022
View reviewed changes
Snowflake.Data/Core/FileTransfer/SFFileTransferAgent.cs

if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
{
location = Path.GetFullPath(location);
location = directoryName + "\\" + fileName;
Copy link
Collaborator

Choose a reason for hiding this comment

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

will \ work on Linux/Mac?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, the connector was able to build and run tests on Linux and Mac machines as well

sfc-gh-igarish
sfc-gh-igarish reviewed Nov 10, 2022
View reviewed changes
Snowflake.Data/Core/FileTransfer/StorageClient/SFS3Client.cs
var task = client.GetObjectAsync(request);
task.Wait();
// Send the request
HttpWebRequest request = (HttpWebRequest)HttpWebRequest.Create(uri.AbsoluteUri);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Previously it's an async operation. Are we getting same behavior after the change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Previously the client.GetObjectAsync() function was used but task.Wait() was synchronously blocking it. So the behaviour is similar since they both execute as synchronous

sfc-gh-igarish
sfc-gh-igarish reviewed Nov 10, 2022
View reviewed changes
Snowflake.Data/Snowflake.Data.csproj Outdated
@@ -38,7 +34,7 @@
<Reference Include="System.Net.Http" />
</ItemGroup>

<ItemGroup Condition="'$(TargetFramework)' == 'netstandard2.0'">
<ItemGroup Condition="'$(TargetFramework)' == 'net6.0'">
Copy link
Collaborator

Choose a reason for hiding this comment

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

why net 6.0 here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've reverted it back, it was just for testing purposes

sfc-gh-igarish
sfc-gh-igarish reviewed Nov 10, 2022
View reviewed changes
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.

Are we going to do same change in other driver? Why we are moving away from SDK and use REST API? Please benchmark before and after change.

@github-actions
Copy link

github-actions bot commented Nov 14, 2022 •
edited
Loading

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@sfc-gh-ext-simba-lf
Copy link
Collaborator

I have read the CLA Document and I hereby sign the CLA

@SimbaGithub
Copy link
Collaborator Author

Are we going to do same change in other driver? Why we are moving away from SDK and use REST API?

This was requested as some of the customer projects also referenced the cloud SDKs which caused incompatibilities in their projects.

Another reason was due to being required to install the entire SDK when only upload/download was being used so it was requested to the REST API instead.

The python connector also uses the REST API

@SimbaGithub
Copy link
Collaborator Author

Please benchmark before and after change.

I will work on benchmarking and update again with the results

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sfc-gh-igarish sfc-gh-igarish sfc-gh-igarish left review comments

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@SimbaGithub @codecov-commenter @sfc-gh-ext-simba-lf @sfc-gh-igarish