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

[Storage] Azure.Storage.Files.DataLake download APIs are in feature parity with Azure.Storage.Blobs #45498

Closed
wants to merge 18 commits into from

Conversation

nickliu-msft
Copy link
Member

Resolves #45418

@github-actions github-actions bot added the Storage Storage Service (Queues, Blobs, Files) label Aug 13, 2024
@nickliu-msft
Copy link
Member Author

nickliu-msft commented Aug 13, 2024

A few things of note:

  1. Should I add new tests for this change? I'm asking cus
    a. I didn't see any tests for the original Read() and ReadAsync()
    b. Since DataLake lives ontop of Blobs, the "actual processing" methods being called are DownloadStreaming() and
    DownloadContent() which have tests already
  2. I did not add ReadTo() and ReadToAsync() since they seem to be already present within DataLake

Let me know thoughts and if I should incorporate anything. Thanks

@azure-sdk
Copy link
Collaborator

API change check

APIView has identified API level changes in this PR and created following API reviews.

Azure.Storage.Files.DataLake

@amnguye
Copy link
Member

amnguye commented Aug 14, 2024

A few things of note:

  1. Should I add new tests for this change? I'm asking cus
    a. I didn't see any tests for the original Read() and ReadAsync()

The tests are here:
https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/storage/Azure.Storage.Files.DataLake/tests/FileClientTests.cs#L3426

b. Since DataLake lives ontop of Blobs, the "actual processing" methods being called are DownloadStreaming() and
DownloadContent() which have tests already

That being true, I would still like the E2E tests, as that follows the current testing standard we do for when we add any new API.

@seanmcc-msft
Copy link
Member

seanmcc-msft commented Aug 14, 2024

@tg-msft and @schaabs, this PR involves some public interface changes, can you take a look?

Copy link
Member

@jaschrep-msft jaschrep-msft left a comment

Choose a reason for hiding this comment

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

Comments on organization and presentation.

@jaschrep-msft
Copy link
Member

Since DataLake lives ontop of Blobs, the "actual processing" methods being called are DownloadStreaming() and
DownloadContent() which have tests already

@nickliu-msft yes, but we are not just testing that the blobs download works correctly, we are testing that our arguments are mapped onto it correctly and our results are properly translated. Ideally, we would have mocking foundation in place to more directly test that, but for now recordings will have to do. (Also though it's good to have some full end to end tests as well so we'd still have some of these even if we were able to go with a mocking route.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Storage Storage Service (Queues, Blobs, Files)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Azure.Storage.Files.DataLake download APIs are not a feature parity with Azure.Storage.Blobs
5 participants