Skip to content

Storage InputStream and OutputStream #5455

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
Merged
sima-zhu merged 15 commits into from
Oct 4, 2019
Merged

Storage InputStream and OutputStream #5455

sima-zhu merged 15 commits into from
Oct 4, 2019

Conversation

@sima-zhu
Copy link
Contributor

@sima-zhu sima-zhu commented Sep 20, 2019

The PR is to abstract storage input and output stream into common layer. Implemented StorageFileInputStream and StorageFileOutputStream. Make BlobInputStream and BlobOutputStream extends the common one.

Todo list:

  1. Rewrite some of the comments.
  2. Extract some common conversion to azure core if any.
  3. Tests

@sima-zhu sima-zhu self-assigned this Sep 20, 2019
@sima-zhu sima-zhu added Client This issue points to a problem in the data-plane of the library. Storage Storage Service (Queues, Blobs, Files) labels Sep 20, 2019
jaschrep-msft
jaschrep-msft reviewed Sep 26, 2019
View reviewed changes
sdk/storage/azure-storage-common/src/main/java/com/azure/storage/common/StorageInputStream.java Outdated
Copy link
Member

@jaschrep-msft jaschrep-msft Sep 26, 2019

Choose a reason for hiding this comment

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

This has been extracted away to work with more than just blobs.

@sima-zhu sima-zhu requested a review from mssfang as a code owner September 30, 2019 01:31
rickle-msft
rickle-msft reviewed Oct 3, 2019
View reviewed changes
...age/azure-storage-blob/src/main/java/com/azure/storage/blob/specialized/BlobInputStream.java
* @throws StorageException An exception representing any error which occurred during the operation.
*/
BlobInputStream(final BlobAsyncClientBase blobClient, final BlobAccessConditions accessCondition)
BlobInputStream(final BlobAsyncClient blobClient, final BlobAccessConditions accessCondition)
Copy link
Contributor

@rickle-msft rickle-msft Oct 3, 2019

Choose a reason for hiding this comment

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

I think this should be just BlobClient. Streams are used in the sync world, so it doesn't make sense to require a customer to pass in an async client, especially since we always block anyway.

Copy link
Contributor Author

@sima-zhu sima-zhu Oct 3, 2019

Choose a reason for hiding this comment

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

Change to sync will dramatically change the PR, which we can not fit the PR before Friday.
Async to Sync client change is an internal implementation change, so it is ok to do a post preview PR if it is needed.

Copy link
Member

@joshfree joshfree Oct 4, 2019

Choose a reason for hiding this comment

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

Can this feedback be addressed by adding a BlobsyncClient overload in a later update?

Copy link
Contributor Author

@sima-zhu sima-zhu Oct 4, 2019

Choose a reason for hiding this comment

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

Only private or protected method and constructor needs to update for this change. Our public APIs do not take client as param. We can simply change to sync instead of adding sync overload.

rickle-msft
rickle-msft reviewed Oct 3, 2019
View reviewed changes
sdk/storage/azure-storage-file/src/main/java/com/azure/storage/file/StorageFileInputStream.java
* @param fileRangeLength How much data the stream should return after fileRangeOffset.
* @throws StorageException An exception representing any error which occurred during the operation.
*/
StorageFileInputStream(final FileAsyncClient fileAsyncClient, long fileRangeOffset, Long fileRangeLength)
Copy link
Contributor

@rickle-msft rickle-msft Oct 3, 2019

Choose a reason for hiding this comment

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

FileAsyncClient [](start = 33, length = 15)

sync client

Copy link
Contributor Author

@sima-zhu sima-zhu Oct 3, 2019

Choose a reason for hiding this comment

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

Prefer no change for this PR before preview

@rickle-msft
Copy link
Contributor

rickle-msft commented Oct 3, 2019 

}

I think if you make commit() an abstract method on the base and then have it be a no op on the file output stream, then you don't have to duplicate any of this. This whole thing can move into the base.

Refers to: sdk/storage/azure-storage-blob/src/main/java/com/azure/storage/blob/specialized/BlobOutputStream.java:80 in 0aa814b. [](commit_id = 0aa814b, deletion_comment = False)

@sima-zhu
Copy link
Contributor Author

sima-zhu commented Oct 3, 2019 

}

I think if you make commit() an abstract method on the base and then have it be a no op on the file output stream, then you don't have to duplicate any of this. This whole thing can move into the base.

Refers to: sdk/storage/azure-storage-blob/src/main/java/com/azure/storage/blob/specialized/BlobOutputStream.java:80 in 0aa814b. [](commit_id = 0aa814b, deletion_comment = False)

@rickle-msft Could you clarify which is the base BlobOutputStream or StorageOutputStream?

I did not find commit() in StorageFileOutputStream, It is an abstract in BlobOutputStream and only used by blockBlobStream. The reason I cannot move to the base is it asks for client which is only available in child stream.

@sima-zhu
Copy link
Contributor Author

sima-zhu commented Oct 3, 2019

/azp run java - storage - tests

@azure-pipelines
Copy link

azure-pipelines bot commented Oct 3, 2019

Azure Pipelines successfully started running 1 pipeline(s).

@sima-zhu sima-zhu requested a review from rickle-msft October 3, 2019 23:53
@sima-zhu
Copy link
Contributor Author

sima-zhu commented Oct 4, 2019

/azp run java - storage - tests

@azure-pipelines
Copy link

azure-pipelines bot commented Oct 4, 2019

Azure Pipelines successfully started running 1 pipeline(s).

@sima-zhu
Copy link
Contributor Author

sima-zhu commented Oct 4, 2019

/azp run java - storage - tests

@azure-pipelines
Copy link

azure-pipelines bot commented Oct 4, 2019

Azure Pipelines successfully started running 1 pipeline(s).

@sima-zhu
Copy link
Contributor Author

sima-zhu commented Oct 4, 2019

/azp run java - storage - tests

@azure-pipelines
Copy link

azure-pipelines bot commented Oct 4, 2019

Azure Pipelines successfully started running 1 pipeline(s).

@sima-zhu sima-zhu merged commit 3c37147 into Azure:master Oct 4, 2019
chrischild pushed a commit to chrischild/azure-sdk-for-java that referenced this pull request Oct 5, 2019
@sima-zhu
Storage InputStream and OutputStream (Azure#5455)
ce3e131
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jaschrep-msft jaschrep-msft jaschrep-msft left review comments

@alzimmermsft alzimmermsft alzimmermsft approved these changes

@gapra-msft gapra-msft Awaiting requested review from gapra-msft

@JonathanGiles JonathanGiles Awaiting requested review from JonathanGiles

@joshfree joshfree Awaiting requested review from joshfree

@mssfang mssfang Awaiting requested review from mssfang

@rickle-msft rickle-msft Awaiting requested review from rickle-msft

Assignees

@sima-zhu sima-zhu

Labels

Client This issue points to a problem in the data-plane of the library. Storage Storage Service (Queues, Blobs, Files)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@sima-zhu @rickle-msft @joshfree @jaschrep-msft @alzimmermsft