Skip to content

Conversation

@samtstern
Copy link
Contributor

bundleStream.close();

// Calling getResult() propagates errors
LoadBundleTaskProgress progress = task.getResult();
Copy link

Choose a reason for hiding this comment

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

Hmm, what happens if we don't have this?

@wu-hui
Copy link

wu-hui commented Mar 12, 2021

Also adding @schmidt-sebastian for another look

@Override
public Task<Query> then(@NonNull Task<LoadBundleTaskProgress> task) throws Exception {
// Close the stream
bundleStream.close();

Choose a reason for hiding this comment

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

Sam - do you think the SDK should close the stream? I am not sure what the expected behavior is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was wondering the same thing ... I personally would like that but I really don't do enough Java I/O programming to have an informed opinion.

@samtstern samtstern merged commit 722ae70 into master Mar 12, 2021
return connection.getInputStream();
}

public void fetchBunldeFrom() throws IOException {
Copy link
Member

Choose a reason for hiding this comment

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

Typo 😁

Suggested change
public void fetchBunldeFrom() throws IOException {
public void fetchBundleFrom() throws IOException {

fun getBundleStream(urlString: String?): InputStream {
val url = URL(urlString)
val connection = url.openConnection() as HttpURLConnection
return connection.inputStream
Copy link
Member

Choose a reason for hiding this comment

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

Does firestore buffer the stream internally? otherwise it may be very inefficient to read from an unbuffered stream

Choose a reason for hiding this comment

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

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants