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

Batch data object fetching for mcclient #123

Merged
merged 4 commits into from
Dec 12, 2016
Merged

Conversation

yusefnapora
Copy link
Contributor

Adds support for the batch get support added in mediachain/concat#100

The mcclient data get command now accepts multiple keys and will fetch them with the batch interface.

Right now it prints the returned objects directly to the console one after another; I'm torn on whether we should wrap them in an array literal first, so that the entire output would be valid JSON.

@yusefnapora yusefnapora force-pushed the yn/mcclient-batch-get branch from 6e586d0 to a82aafd Compare December 8, 2016 22:34
@vyzo
Copy link
Contributor

vyzo commented Dec 9, 2016

It fails to handle null data objects correctly -- these are returned by the batch interface when the object is not found in the datastore:

$ cat /tmp/non-existent-key 
QmeBkfxcaBfA9pvzivRwhF2PM7sXpp4HHQbp7jfTkRCWEa
$ mcclient getData QmeBkfxcaBfA9pvzivRwhF2PM7sXpp4HHQbp7jfTkRCWEa
First argument must be a string, Buffer, ArrayBuffer, Array, or array-like object.

})
stream.on('error', (err) => {
stdout.write(`}\n`)
reject(err)
Copy link
Contributor

Choose a reason for hiding this comment

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

will this print the actual error in stderr or something similar? It would be nice not to swallow dem errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it will print the error message to stderr if you reject a promise, and set the exit status code to 1. It's part of the subcommand helper thing.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, cool, this is precisely the desired behaviour.

stdout.write(`}\n`)
reject(err)
})
stream.on('end', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

this seems unnecessary -- or is the code reused from the data protocol?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we need to resolve the promise at the end of the stream, so we can close the ssh tunnel if it exists and end the process

Copy link
Contributor

Choose a reason for hiding this comment

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

is there something emitting an 'end' object on EOF?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, the fetch request body stream will send the end event once you've consumed the whole thing.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok cool -- I didn't know that.

@yusefnapora yusefnapora merged commit f87fe86 into master Dec 12, 2016
@yusefnapora yusefnapora deleted the yn/mcclient-batch-get branch December 12, 2016 16:51
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.

2 participants