Add stream support in storage clients (Closes #1702)#1703
Conversation
1. Extend BlobClient to support streams 2. Change StorageBackend clients with getStream and putStream methods 3. Update tests for streams in StorageBackends.spec.js
brollb
left a comment
brollb
left a comment
There was a problem hiding this comment.
Nice progress. I left most of my comments inline. A couple remaining questions/comments:
- It would probably be better to make the default methods for getting/putting files to use streams. That is, it would be better to have
putFileandputFileBuffer. - On that front, it might be good to simply remove
getFile. The important thing is that we do not want people using these APIs unless they absolutely have to (ie, streams aren't supported because they are in the browser).
src/common/StreamBlobClient.js
Outdated
| StorageHelpers | ||
| ) { | ||
| const StreamBlobClient = function (logger) { | ||
| const params = this.getBlobClientParams(logger); |
There was a problem hiding this comment.
As this is subclassing BlobClient and the hope is to contribute this back to webgme-engine, this should not modify the method signature unnecessarily. The creation of the blob client parameters should be handled by the storage adapter itself.
| 'client/logger', | ||
| 'deepforge/gmeConfig' | ||
| 'deepforge/gmeConfig', | ||
| 'deepforge/StorageHelpers' |
There was a problem hiding this comment.
If this is going to be refactored, StorageHelpers should be in deepforge/storage.
| throw new Error(`stat not implemented for ${this.name}`); | ||
| }; | ||
|
|
||
| StorageClient.prototype.checkStreamsInBrowser = async function() { |
There was a problem hiding this comment.
Personally, I would rename this to something like ensureStreamsSupported or ensureIsBrowser(msg). This doesn't actually check streams and it appears that streaming in the browser is partially supported (getStream appears to be supported).
There was a problem hiding this comment.
Also, this shouldn't be async
| @@ -1,4 +1,4 @@ | |||
| describe('GME Storage Adapter', function() { | |||
| describe.skip('GME Storage Adapter', function() { | |||
|
I just merged #1710 into master. Could you merge master in (and resolve the merge conflicts)? |
|
Sure. |
1. Move StorageHelpers.js to the deepforge/storage 2. Change StreamBlobClient as an ES6 class without changing constructor signature 3. Change method checkStreamsInBrowser to EnsureStreamSupport(synchronous) 4. Add a StringWritable class in storageBackends.spec.js and verify content in memory 5. Don't skip the GMEStorage unit test as its prototype has getBlobClientParams
|
This now depends on webgme/webgme-engine#215 This will remove the usage of a modified |
|
This is looking good. Could you check out the source of the failing tests? Are they an issue with sciserver or with our request to sciserver? |
|
Main concerning failing tests: |
|
These tests are a passing for me. |
2 participants
From Commit Logs: