This repository has been archived by the owner on Oct 30, 2018. It is now read-only.
Isomorphic BridgeClient #628
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Working on making bridge-client isomorphic.
Conflicts: lib/bridge-client/index.js
|
This PR has several changes wound up into one.
|
|
@dylanlott is finishing up some of the changes to |
…tt/core into isomorphic-bridge-client
added 3 commits
January 20, 2017 20:57
retrohacker
commented
Jan 23, 2017
lib/bridge-client/blacklist.js
Outdated
| // Cleanup stale references and return the file | ||
| return cb(null, self._reap(self.blacklist)); | ||
| }); | ||
| rs.pipe(cs); |
There was a problem hiding this comment.
Do we need to handle an error on cs?
retrohacker
commented
Jan 23, 2017
lib/bridge-client/index.js
Outdated
| * @param {String} [options.blacklistFolder] - The folder that blacklist | ||
| * entries will be persisted to if using the default fs-store | ||
| * @param {Object} [options.store] - The store that blacklist enteries will be | ||
| * persisted to if using the default fs-store |
There was a problem hiding this comment.
Documentation is borked after merge.
retrohacker
commented
Jan 23, 2017
| 'Supplied store must implement abstract-blob-store'); | ||
|
|
||
| assert.ok(typeof options.store.createReadStream === 'function', | ||
| 'Supplied store must implement abstract-blob-store'); |
There was a problem hiding this comment.
We also need to check for .exists() which will be used in UploadState.cleanup
retrohacker
commented
Jan 23, 2017
lib/bridge-client/index.js
Outdated
| } | ||
|
|
||
| if (file instanceof Readable) { | ||
| var ws = self._store.createWriteStream(fileName); |
There was a problem hiding this comment.
Do we need to handle the error event on ws?
retrohacker
commented
Jan 23, 2017
lib/bridge-client/index.js
Outdated
| @@ -493,27 +580,28 @@ BridgeClient.prototype._shardUploadWorker = function(task, done) { | |||
| * @param {Object} frame - Frame object returned from bridge | |||
| * @param {UploadState} state - The upload state machine | |||
There was a problem hiding this comment.
We need to update the method signatures to match the new options object.
retrohacker
commented
Jan 23, 2017
lib/file-handling/file-demuxer.js
Outdated
| //turn filePath into a readable Stream | ||
| if (typeof filePath === 'string') { | ||
| options.fileSize = fs.statSync(filePath).size; | ||
| filePath = fs.createReadStream(filePath); |
There was a problem hiding this comment.
We need to handle errors on filePath if we weren't passed a store.
There was a problem hiding this comment.
If we are passed a store, the user should be handling errors.
retrohacker
commented
Jan 23, 2017
package.json
Outdated
| @@ -16,7 +16,7 @@ | |||
| "build": "./node_modules/.bin/browserify index.js -s storj -o dist/storj.browser.js", | |||
| "make-docs": "mkdir -p ./jsdoc && rm -r ./jsdoc && ./node_modules/.bin/jsdoc index.js lib -r -R README.md -u ./doc -c .jsdoc.json --verbose -d ./jsdoc && cp -r doc/assets jsdoc/assets", | |||
| "publish-docs": "npm run make-docs && node script/publishdoc.js" | |||
| }, | |||
| }, | |||
There was a problem hiding this comment.
Extra whitespace
tacticalchihuahua
previously requested changes
Jan 23, 2017
There was a problem hiding this comment.
Preliminary review shows many style guide inconsistencies. Be sure to have read https://github.com/Storj/core/blob/master/CONTRIBUTING.md#style-guide and follow it.
lib/bridge-client/blacklist.js
Outdated
| * @param {Object} options.logger - Logger instance | ||
| * @param {Object} options.store - The store that blacklist enteries will be | ||
| * persisted to. This object must be compatible with the API of | ||
| * [abstract-blob-store](https://github.com/maxogden/abstract-blob-store) |
There was a problem hiding this comment.
For links in JSDoc comments, use {@link https://github.com/maxogden/abstract-blob-store|abstract-blob-store} to render it properly on the docs sits.
lib/bridge-client/blacklist.js
Outdated
| var self = this; | ||
|
|
||
| // If we already have a blacklist, return it | ||
| if(self.blacklist !== undefined) { |
There was a problem hiding this comment.
Keep consistent with style conventions - always add space between if and ().
lib/bridge-client/blacklist.js
Outdated
| } | ||
| Blacklist.prototype._saveToStore = function(cb) { | ||
| var ws = this._store.createWriteStream(this.blacklistKey); | ||
| ws.end(JSON.stringify(this.blacklist), function (e) { |
There was a problem hiding this comment.
Style, remove space between function and (e).
lib/bridge-client/blacklist.js
Outdated
| var ws = this._store.createWriteStream(this.blacklistKey); | ||
| ws.end(JSON.stringify(this.blacklist), function (e) { | ||
| // Throw to maintain compatibility with fs.writeFileSync | ||
| if(e) { throw e; } |
There was a problem hiding this comment.
Style, space between if and (e) and always use a newline for blocks.
lib/bridge-client/blacklist.js
Outdated
| var self = this; | ||
| var rs = self._store.createReadStream(self.blacklistKey); | ||
| // If the file doesn't exist, return an empty object. | ||
| rs.on('error', function () { |
There was a problem hiding this comment.
Style: remove space between function and ()
test/bridge-client/index.unit.js
Outdated
| @@ -681,7 +728,12 @@ describe('BridgeClient', function() { | |||
| it('should return error if file is unsupported size', function(done) { | |||
| var StubbedClient = proxyquire('../../lib/bridge-client', { | |||
| fs: { | |||
| statSync: sinon.stub().returns({ size: 0 }) | |||
| createReadStream: function () { | |||
There was a problem hiding this comment.
Style, remove space between function and (...)
test/bridge-client/index.unit.js
Outdated
| statSync: sinon.stub().returns({ size: 0 }) | ||
| createReadStream: function () { | ||
| var rs = new Readable(); | ||
| rs._read = function () {}; |
There was a problem hiding this comment.
Style, remove space between function and (...)
test/bridge-client/index.unit.js
Outdated
| expect(_retry.called).to.equal(true); | ||
| done(); | ||
| // Make sure callback is triggered to avoid race | ||
| client._blacklist.push('foo', function () { |
There was a problem hiding this comment.
Style, remove space between function and (...)
test/bridge-client/index.unit.js
Outdated
| } | ||
| var StubbedClient = proxyquire('../../lib/bridge-client', {}); | ||
|
|
||
| function createReadStream () { |
There was a problem hiding this comment.
Style, remove space between function functionName and (...)
test/bridge-client/index.unit.js
Outdated
| return this.push(null); | ||
| } | ||
| var StubbedClient = proxyquire('../../lib/bridge-client', {}); | ||
| function createReadStream () { |
There was a problem hiding this comment.
Style, remove space between function functionName and (...)
…tt/core into isomorphic-bridge-client
tacticalchihuahua
approved these changes
Feb 13, 2017
added 2 commits
February 13, 2017 15:54
retrohacker
commented
Feb 13, 2017
lib/file-handling/file-demuxer.js
Outdated
| } | ||
|
|
||
| // TODO: pass size into constructor for file | ||
| // TODO for this._source pass in readableStream from blob store |
There was a problem hiding this comment.
Both of these TODOs have been done. Remove before merge. LONG TERM TODO can be removed in favor of opening an issue.
| } | ||
| }); | ||
| async.each(this.cleanQueue, function(options, cb) { | ||
| options.store.exists(options.key, function(err, exists) { |
There was a problem hiding this comment.
Do we set options.key anywhere? Looks like we are leaving the temporary files behind.
There was a problem hiding this comment.
key is set in bridge-client/index.js
lib/bridge-client/index.js
Outdated
| return done(err); | ||
| } | ||
| return _shardTransferComplete(); | ||
| }); | ||
| }; | ||
|
|
||
| /** | ||
| * Transfers a shard to a specified farmer | ||
| * @private | ||
| * @param {events.EventEmitter} emitter - For getting status events |
|
|
||
| self._logger.info('Hash for this shard is: %s', hash); | ||
|
|
||
| function _handleError(err) { | ||
| self._logger.warn('Failed to upload shard...'); | ||
| state.cleanup(); | ||
| return state.callback(err); | ||
| state.cleanup(function(err2){ |
There was a problem hiding this comment.
Does err2 contain all failures or only one?
There was a problem hiding this comment.
It only includes the most recent error. This should be acceptable for now.
There was a problem hiding this comment.
Note though, an error here means that we were unable to remove a file from a blobstore for whatever reason.
lib/bridge-client/index.js
Outdated
| var self = this; | ||
| var fileSize = fs.statSync(file).size; | ||
| var fileHash = crypto.randomBytes(6).toString('hex'); |
There was a problem hiding this comment.
Calling this a hash might be misleading.
tacticalchihuahua
changed the title
|
@retrohacker awesome, will run this through integration and confirm no regressions. if all is good it will get merged tomorrow! 🍺 |
|
@retrohacker regression on uploading/downloading files larger than a single shard. (https://github.com/retrohacker/core/blob/master/lib/utils.js#L504). I was, however, able to upload single-shard files and download single-shard files. Uploading Multi-shard FileDownloading Multi-shard File |
tacticalchihuahua
suggested changes
Feb 14, 2017
|
Ah, something weird is happening in the transform stream, seems like |
|
Confirmed repro on downloading and uploading. Though uploading seems inconsistent. Sometimes it works, and sometimes it throws this err where |
|
It seems the calculated hash for each file is different which is also triggering a new err coming from |
|
@bookchin @retrohacker latest commit should fix things. It seems we were simply not creating a new random number for each new shard. |
|
@nginnever I can confirm that large downloads are now working, however I'm still not able to upload large files: It looks like we are having some trouble with the bridge, but I can still upload single shard files: That particular error, I followed back and found that it's not related to your changes, but rather it's a bug in the uploader utility where if nothing is written to the transform stream before ending, you'll get that null reference. The reason that is happening lies between the issues we already knew we had with the state management/retry logic that needs to be rewritten and the current production issues we are having with the bridge. So all of that said, I'm good to merge this PR. Excellent work! 🎉 |
tacticalchihuahua
approved these changes
Feb 19, 2017
|
Will tag v6.2.0 shortly |
|
🎉 🌮 🍻 🎉 |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Supersedes:
#626 and #627