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

Added request options for DataSource retry info #970

Merged
merged 2 commits into from
Jan 23, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 0 additions & 10 deletions lib/HttpDataSource.js

This file was deleted.

4 changes: 2 additions & 2 deletions lib/Model.js
Original file line number Diff line number Diff line change
Expand Up @@ -355,7 +355,7 @@ Model.prototype._hasValidParentReference = require("./deref/hasValidParentRefere
* @param {Path} path - the path to retrieve
* @return {Observable.<*>} - the value for the path
* @example
var model = new falcor.Model({source: new falcor.HttpDataSource("/model.json") });
var model = new falcor.Model({source: new HttpDataSource("/model.json") });

model.
getValue('user.name').
Expand All @@ -373,7 +373,7 @@ Model.prototype.getValue = require("./get/getValue");
* @param {Object} value - the value to set
* @return {Observable.<*>} - the value for the path
* @example
var model = new falcor.Model({source: new falcor.HttpDataSource("/model.json") });
var model = new falcor.Model({source: new HttpDataSource("/model.json") });

model.
setValue('user.name', 'Jim').
Expand Down
4 changes: 3 additions & 1 deletion lib/request/GetRequestV2.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,17 @@ var InvalidSourceError = require("./../errors/InvalidSourceError");
*
* @param {Scheduler} scheduler -
* @param {RequestQueueV2} requestQueue -
* @param {number} attemptCount
*/
var GetRequestV2 = function(scheduler, requestQueue) {
var GetRequestV2 = function(scheduler, requestQueue, attemptCount) {
this.sent = false;
this.scheduled = false;
this.requestQueue = requestQueue;
this.id = ++REQUEST_ID;
this.type = GetRequestType;

this._scheduler = scheduler;
this._attemptCount = attemptCount;
this._pathMap = {};
this._optimizedPaths = [];
this._requestedPaths = [];
Expand Down
23 changes: 18 additions & 5 deletions lib/request/RequestQueueV2.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,24 +29,32 @@ RequestQueueV2.prototype = {
* currently could be batched potentially in the future. Since no batching
* is required the setRequest action is simplified significantly.
*
* @param {JSONGraphEnvelope) jsonGraph -
* @param {JSONGraphEnvelope} jsonGraph -
* @param {number} attemptCount
* @param {Function} cb
*/
set: function(jsonGraph, cb) {
set: function(jsonGraph, attemptCount, cb) {
if (this.model._enablePathCollapse) {
jsonGraph.paths = falcorPathUtils.collapse(jsonGraph.paths);
}

return sendSetRequest(jsonGraph, this.model, cb);
if (cb === undefined) {
cb = attemptCount;
attemptCount = undefined;
}

return sendSetRequest(jsonGraph, this.model, attemptCount, cb);
},

/**
* Creates a get request to the dataSource. Depending on the current
* scheduler is how the getRequest will be flushed.
* @param {Array} requestedPaths -
* @param {Array} optimizedPaths -
* @param {number} attemptCount
* @param {Function} cb -
*/
get: function(requestedPaths, optimizedPaths, cb) {
get: function(requestedPaths, optimizedPaths, attemptCount, cb) {
var self = this;
var disposables = [];
var count = 0;
Expand All @@ -57,6 +65,11 @@ RequestQueueV2.prototype = {
var disposed = false;
var request;

if (cb === undefined) {
cb = attemptCount;
attemptCount = undefined;
}

for (i = 0, len = requests.length; i < len; ++i) {
request = requests[i];
if (request.type !== RequestTypes.GetRequest) {
Expand Down Expand Up @@ -99,7 +112,7 @@ RequestQueueV2.prototype = {
// After going through all the available requests if there are more
// paths to process then a new request must be made.
if (oRemainingPaths && oRemainingPaths.length) {
request = new GetRequest(self.scheduler, self);
request = new GetRequest(self.scheduler, self, attemptCount);
requests[requests.length] = request;
++count;
var disposable = request.batch(rRemainingPaths, oRemainingPaths, refCountCallback);
Expand Down
2 changes: 1 addition & 1 deletion lib/request/flushGetRequest.js
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ module.exports = function flushGetRequest(request, pathSetArrayBatch, callback)
// we cancel at the callback above.
var getRequest;
try {
getRequest = model._source.get(requestPaths);
getRequest = model._source.get(requestPaths, request._attemptCount);
} catch (e) {
callback(new InvalidSourceError());
return null;
Expand Down
5 changes: 3 additions & 2 deletions lib/request/sendSetRequest.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,10 @@ var emptyDisposable = {dispose: function() {}};
* @private
* @param {JSONGraphEnvelope} jsonGraph -
* @param {Model} model -
* @param {number} attemptCount
* @param {Function} callback -
*/
var sendSetRequest = function(originalJsonGraph, model, callback) {
var sendSetRequest = function(originalJsonGraph, model, attemptCount, callback) {
var paths = originalJsonGraph.paths;
var modelRoot = model._root;
var errorSelector = modelRoot.errorSelector;
Expand All @@ -28,7 +29,7 @@ var sendSetRequest = function(originalJsonGraph, model, callback) {
var setObservable;
try {
setObservable = model._source.
set(originalJsonGraph);
set(originalJsonGraph, attemptCount);
} catch (e) {
callback(new InvalidSourceError());
return emptyDisposable;
Expand Down
2 changes: 1 addition & 1 deletion lib/response/get/getRequestCycle.js
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ module.exports = function getRequestCycle(getResponse, model, results, observer,
}

var currentRequestDisposable = requestQueue.
get(boundRequestedMissingPaths, optimizedMissingPaths, function(err, data, hasInvalidatedResult) {
get(boundRequestedMissingPaths, optimizedMissingPaths, count, function(err, data, hasInvalidatedResult) {
if (model._treatDataSourceErrorsAsJSONGraphErrors ? err instanceof InvalidSourceError : !!err) {
if (results.hasValues) {
observer.onNext(results.values && results.values[0]);
Expand Down
2 changes: 1 addition & 1 deletion lib/response/set/SetResponse.js
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ SetResponse.prototype._subscribe = function _subscribe(observer) {

// Starts the async request cycle.
return setRequestCycle(
model, observer, groups, isJSONGraph, isProgressive, 0);
model, observer, groups, isJSONGraph, isProgressive, 1);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tquetano-netflix The initial value was changed here, in coordination with the retry logic condition.

};

/**
Expand Down
4 changes: 2 additions & 2 deletions lib/response/set/setRequestCycle.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ module.exports = function setRequestCycle(model, observer, groups,
var requestedPaths = requestedAndOptimizedPaths.requestedPaths;

// we have exceeded the maximum retry limit.
if (count === model._maxRetries) {
if (count > model._maxRetries) {

Choose a reason for hiding this comment

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

Just asking considering the original comparison logic, should this be >=?

Copy link
Contributor Author

@jcranendonk jcranendonk Jan 23, 2020

Choose a reason for hiding this comment

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

I made the change to align the retry counts with getRequestCycle; one started at 0, the other at 1. Now they both start at 1, which lines up better with the Zuul retry header value.

observer.onError(new MaxRetryExceededError(optimizedPaths));
return {
dispose: function() {}
Expand Down Expand Up @@ -59,7 +59,7 @@ module.exports = function setRequestCycle(model, observer, groups,
// If disposed before this point then the sendSetRequest will not
// further any callbacks. Therefore, if we are at this spot, we are
// not disposed yet.
set(currentJSONGraph, function(error, jsonGraphEnv) {
set(currentJSONGraph, count, function(error, jsonGraphEnv) {
if (error instanceof InvalidSourceError) {
observer.onError(error);
return;
Expand Down
5 changes: 0 additions & 5 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

67 changes: 32 additions & 35 deletions test/data/LocalDataSource.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
var Rx = require("rx");
var Observable = Rx.Observable;
var falcor = require("./../../lib/");
var _ = require("lodash");
var noOp = function(a, b, c) { return c; };
const Rx = require("rx");
const falcor = require("./../../lib/");
const _ = require("lodash");
const noOp = function(a, b, c) { return c; };

var LocalSource = module.exports = function(cache, options) {
const LocalSource = module.exports = function(cache, options) {
this._options = _.extend({
miss: 0,
onGet: noOp,
Expand All @@ -14,42 +13,41 @@ var LocalSource = module.exports = function(cache, options) {
materialize: false
}, options);
this._missCount = 0;
this.model = new falcor.Model({cache: cache});
this.model = new falcor.Model({cache});

if (this._options.materialize) {
this.model = this.model._materialize();
}
};

LocalSource.prototype = {
setModel: function(modelOrCache) {
setModel(modelOrCache) {
if (modelOrCache instanceof falcor.Model) {
this.model = modelOrCache;
} else {
this.model = new falcor.Model({cache: modelOrCache});
}
},
get: function(paths) {
var self = this;
var options = this._options;
var miss = options.miss;
var onGet = options.onGet;
var onResults = options.onResults;
var wait = options.wait;
var errorSelector = options.errorSelector;
return Rx.Observable.create(function(observer) {
get(paths, dsRequestOpts) {
const self = this;
const options = this._options;
const miss = options.miss;
const onGet = options.onGet;
const onResults = options.onResults;
const wait = options.wait;
const errorSelector = options.errorSelector;
return Rx.Observable.create(observer => {
function exec() {
var results;
var values = [{}];
const values = [{}];
if (self._missCount >= miss) {
onGet(self, paths);
onGet(self, paths, dsRequestOpts);
self.model._getPathValuesAsJSONG(self.model, paths, values, errorSelector);
} else {
self._missCount++;
}

// always output all the paths
var output = {
const output = {
// paths: paths,
jsonGraph: {}
};
Expand All @@ -68,21 +66,20 @@ LocalSource.prototype = {
}
});
},
set: function(jsongEnv) {
var self = this;
var options = this._options;
var miss = options.miss;
var onSet = options.onSet;
var onResults = options.onResults;
var wait = options.wait;
var errorSelector = options.errorSelector;
return Rx.Observable.create(function(observer) {
set(jsongEnv, dsRequestOpts) {
const self = this;
const options = this._options;
const onSet = options.onSet;
const onResults = options.onResults;
const wait = options.wait;
const errorSelector = options.errorSelector;
return Rx.Observable.create(observer => {
function exec() {
var seed = [{}];
var tempModel = new falcor.Model({
const seed = [{}];
const tempModel = new falcor.Model({
cache: jsongEnv.jsonGraph,
errorSelector: errorSelector});
jsongEnv = onSet(self, tempModel, jsongEnv);
errorSelector});
jsongEnv = onSet(self, tempModel, jsongEnv, dsRequestOpts);

tempModel.set(jsongEnv).subscribe();
tempModel._getPathValuesAsJSONG(
Expand All @@ -103,7 +100,7 @@ LocalSource.prototype = {
}
});
},
call: function(path, args, suffixes, paths) {
call(path, args, suffixes, paths) {
return Rx.Observable.empty();
}
};
Expand Down
Loading