-
Notifications
You must be signed in to change notification settings - Fork 97
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
FLUID-5542: Queued DataSource #566
base: main
Are you sure you want to change the base?
Changes from 4 commits
8a8e11c
fe6a81e
a29872f
983ff2f
09b61af
1bdbf25
fed923a
53d355b
0550ea1
0613710
a124dce
6b19c80
edacdcf
80c1e6e
b1c3544
27532db
f07ec54
ac6d291
bb4fab5
05a5a22
f8865db
942976a
c62458d
a4440df
5020fb2
9a857bf
454ea1c
c5ae8ec
fc2a1d6
21803c5
b07f492
88c0e26
b395ca8
0e5fc60
550f145
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -343,5 +343,197 @@ var fluid_2_0 = fluid_2_0 || {}; | |
return fluid.NO_VALUE; | ||
}; | ||
|
||
/** Start dataSource **/ | ||
|
||
/* | ||
* A grade definining an emptyDataSource | ||
* The primary reason for this grade is to provide details on the | ||
* expected structure a DataSource should have. | ||
*/ | ||
fluid.defaults("fluid.emptyDataSource", { | ||
gradeNames: ["fluid.eventedComponent", "autoInit"] | ||
// Invokers should be defined for the typical HTTP rest requests. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Whilst REST is one particular concretisation of a dataSource, we should be clear that there are others. For example, we may be writing to cookies, or an indexedDB database, or a git repository etc. There may or may not be particular REST-like HTTP calls involved to back up the implementation of the DataSource. |
||
// Each method should have the signature (directModel, callback) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This isn't quite correct - the signature of the "set" method is (directModel, model, callback) where "model" holds the data to be written. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought model was inside of directModel. Do you have documentation or an example of the what should be contained within directModel and more details about the structure of a dataSource in general. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's some commentary on this page http://wiki.fluidproject.org/display/fluid/Notes+on+Kettle - at the heading "DATASOURCES AND URLS WITHIN KETTLE" |
||
// | ||
// directmodel is a JSON object cotaining the directives and payload | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This isn't clear or correct - directModel holds "coordinate" information, describing where the data which is being addressed is to be read or written from. It doesn't include the payload to the request, this appears in "model" |
||
// to the request. (e.g. {model: {key: value}} ) | ||
// | ||
// callback is a function that will be called after the request has | ||
// returned. | ||
// | ||
// invokers: { | ||
// "get": {}, | ||
// "set": {}, // set should handle POST and PUT requests | ||
// "delete": {} | ||
// } | ||
}); | ||
|
||
/* | ||
* A basic request queue that will execute each request, one-by-one | ||
* in the order that they are received. | ||
*/ | ||
fluid.defaults("fluid.requestQueue", { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since this contains a definition of the "add" operation, it should be given a specialised name such as "fluid.requestQueue.fifo" and/or split into two grades, with the "fluid.requestQueue" reserved for a queue without a policy |
||
gradeNames: ["fluid.standardRelayComponent", "autoInit"], | ||
events: { | ||
queued: null, | ||
unqueued: null | ||
}, | ||
model: { | ||
isActive: false | ||
}, | ||
members: { | ||
queue: [] | ||
}, | ||
listeners: { | ||
"unqueued": "{that}.start", | ||
"queued": "{that}.start" | ||
}, | ||
invokers: { | ||
add: { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This invoker should probably be named "queue" since it doesn't necessarily "add" (purely) a request - it may "add" by displacing an existing request in which case the name "add" would seem confusing - since there would be the same number of requests in the queue after the operation has completed |
||
funcName: "fluid.requestQueue.add", | ||
args: ["{that}", "{arguments}.0"] | ||
}, | ||
start: { | ||
funcName: "fluid.requestQueue.start", | ||
args: ["{that}"] | ||
} | ||
} | ||
}); | ||
|
||
/* | ||
* Adds reqeusts to the queue in the order they are received. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. typo here |
||
* | ||
* The request object contains the request function and arguments. | ||
* In the form {method: requestFn, directModel: {}, callback: callbackFn} | ||
*/ | ||
fluid.requestQueue.add = function (that, request) { | ||
that.queue.push(request); | ||
that.events.queued.fire(request); | ||
}; | ||
|
||
fluid.requestQueue.start = function (that) { | ||
if (!that.model.isActive && that.queue.length) { | ||
var request = that.queue.shift(); | ||
var callbackProxy = function () { | ||
that.applier.change("isActive", false); | ||
that.events.unqueued.fire(request); | ||
request.callback.apply(null, arguments); | ||
}; | ||
|
||
that.applier.change("isActive", true); | ||
request.method(request.directModel, callbackProxy); | ||
} | ||
}; | ||
|
||
/* | ||
* A request queue that will only store the latests request in the queue. | ||
* Intermediate requests that are queued but not invoked, will be replaced | ||
* by new ones. | ||
*/ | ||
fluid.defaults("fluid.requestQueue.debounce", { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This queue is misleadingly named - traditionally, "debouncing" involves the application of some kind of time window. Consult this link for the detailed difference between debouncing and throttling - http://unscriptable.com/2009/03/20/debouncing-javascript-methods/ - we should make sure that any names we use don't give false clues as to what kind of implementation we have. This might be called "amalgamate" since the expectation with a dataSource is that later requests include all the changes in earlier ones as well as some more - and so the behaviour is to coalesce/amalgamate multiple successive change requests into a single one that holds all of the most recent changes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had been following this example http://benalman.com/projects/jquery-throttle-debounce-plugin/ but I'm not sure I fully understand your comment, particularly about the amalgamation. I don't think there are currently any provisions to ensure that requests are merged together. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can see that in that source also, "debounce" accepts a time threshold for coalescing multiple calls together - our version here doesn't. We don't make any explicit provision to merge requests together, but given this is a dataSource wrapper rather than just a general "processor of signals" (as those other source impls are) this is the expected usage pattern of the wrapper. Simply by virtue of being a wrapper of the dataSource API, rather than a wrapper for a general sequence of function calls, there will be a natural semantic induced of "coalescing or amalgamating" the requests - so our naming could usefully reflect that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @amb26 i think i understand why you want to reanimate it to something other than debounce and add in some merging. What I don't understand is how to do this. Do you have a suggestions? Keep in mind that the writeQueue handles both set and delete calls which have different signatures. Also how would we keep the directModel straight if they are being merged. I have a feeling that I'm missing something here that would make this all work. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jobara - we wouldn't do anything special - just to consolidate our implementations around particular concrete use cases. I think @the-t-in-rtf 's case simply requires "at most one request in flight with all but the most recent discarded - but do not issue any requests at all if they are continuing to arrive at a certain rate" and unless I am mistaken I think ours requires the same, only minus the delay criterion. That is, I think we have an interest in opportunistically writing to persistence if we have been required to, but simply ensuring we don't have more than one outstanding request at a time. Given @the-t-in-rtf 's case is related to UI liveness rather than persistence, I think there is reason to supply the "debounce" in his case. So I think we have two main varieties required - i) simple concurrency limiting (for writing to couch/pouch) in both cases, older requests are discarded to make way for newer ones, if a request is "not queued" Does this seem right? Can you think of other requirements or problems? |
||
gradeNames: ["fluid.requestQueue", "autoInit"], | ||
invokers: { | ||
add: { | ||
funcName: "fluid.requestQueue.debounce.add", | ||
args: ["{that}", "{arguments}.0"] | ||
} | ||
} | ||
}); | ||
|
||
/* | ||
* Adds only one item to the queue at a time, new requests replace older ones | ||
* | ||
* The request object contains the request function and arguments. | ||
* In the form {method: requestFn, directModel: {}, callback: callbackFn} | ||
*/ | ||
fluid.requestQueue.debounce.add = function (that, request) { | ||
that.queue[0] = request; | ||
that.events.queued.fire(request); | ||
}; | ||
|
||
/* | ||
* A request queue that will only queue requests that are received after | ||
* a specified delay (in milliseconds). | ||
*/ | ||
fluid.defaults("fluid.requestQueue.throttle", { | ||
gradeNames: ["fluid.requestQueue", "autoInit"], | ||
delay: 10, // delay in milliseconds | ||
model: { | ||
isThrottled: false | ||
}, | ||
invokers: { | ||
add: { | ||
funcName: "fluid.requestQueue.throttle.add", | ||
args: ["{that}", "{arguments}.0"] | ||
} | ||
} | ||
}); | ||
|
||
/* | ||
* Adds items to the queue after a specified delay. | ||
* | ||
* The request object contains the request function and arguments. | ||
* In the form {method: requestFn, directModel: {}, callback: callbackFn} | ||
*/ | ||
fluid.requestQueue.throttle.add = function (that, request) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure this strategy implements a semantic that is widely useful. Typically someone who is interested in throttling is probably also interested in "recency" - that is, they would like to make sure that the most recent request which is sent is actually acted on rather than thrown away as in the current impl. The impl should make sure that either i) it never discards requests, or ii) if it discards requests, it discards older ones rather than newer ones There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. so you that this should be more like debounce, where during the time limit the requests are debounced and then after that, they are added? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we keep it at all - we should make sure that we only implement functions or components where we immediately have a use case amongst ourselves or our partners. I think we currently have two immediate use cases on the table - i) limiting write requests to a source of persistence that may self-conflict (e.g. Couch/Pouch), and ii) limiting read requests which are issued to produce a responsive autocomplete API. We shouldn't implement more queue variants than we can demonstrate an immediate use case for, or else we may end up implementing "white elephants" that don't end up with uses. If you can produce a user with a use case for the "throttle" variant, you can consult them for what semantic they want - otherwise we should most likely drop it. I was only reflecting in the comment that the semantic "didn't seem likely to be useful" - but the acid test for this is finding a user to whom it is useful :) |
||
if (!that.model.isThrottled) { | ||
that.applier.change("isThrottled", true); | ||
that.queue.push(request); | ||
that.events.queued.fire(request); | ||
setTimeout(function () { | ||
that.applier.change("isThrottled", false); | ||
}, that.options.delay); | ||
} | ||
}; | ||
|
||
/* | ||
* A dataSource wrapper providing a queuing mechanism for requests. | ||
* The queue subcomponents, writeQueue (set/delete) and readQueue (get) | ||
* can be configured to use any of the request queue grades. | ||
* | ||
* A fully implemented dataSource, following the structure oulined by fluid.emptyDataSource, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. typo here |
||
* must be provided in the wrappedDataSource subcomponent. The get, set, and delete methods | ||
* found on the queuedDataSource will call thier counterparts in the wrappedDataSource, after | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. typo here |
||
* filtering through the appropriate queue. | ||
*/ | ||
fluid.defaults("fluid.queuedDataSource", { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The naming is still confusing here. The comment above suggests that this is some kind of generic component that permits different queue policies - but in fact this grade is already bound to an implementation that operates a particular hardcoded policy that enforces a queue size of 1 (via "fluid.queuedDataSource.enqueueImpl"). This top-level grade should be better factored into a generic (possibly abstract) grade that doesn't commit the user to anything, with functions like enqueueImpl more clearly named (bound in a different grade/namespace) to emphasize that they are part of a particular concrete realisation of the queuedDataSource. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See above comment There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. created a new grade called debouncedDatasource that attaches the enqueue methods. |
||
gradeNames: ["fluid.standardRelayComponent", "autoInit"], | ||
components: { | ||
writeQueue: { | ||
type: "fluid.requestQueue" | ||
}, | ||
readQueue: { | ||
type: "fluid.requestQueue" | ||
}, | ||
wrappedDataSource: { | ||
// requires a dataSource that implements the standard set, get, and delete methods. | ||
type: "fluid.emptyDataSource" | ||
} | ||
}, | ||
invokers: { | ||
set: { | ||
funcName: "fluid.queuedDataSource.set", | ||
args: ["{writeQueue}", "{wrappedDataSource}.set", "{arguments}.0", "{arguments}.1"] | ||
}, | ||
get: { | ||
funcName: "fluid.queuedDataSource.set", | ||
args: ["{readQueue}", "{wrappedDataSource}.get", "{arguments}.0", "{arguments}.1"] | ||
}, | ||
"delete": { | ||
funcName: "fluid.queuedDataSource.set", | ||
args: ["{writeQueue}", "{wrappedDataSource}.delete", "{arguments}.0", "{arguments}.1"] | ||
} | ||
} | ||
}); | ||
|
||
fluid.queuedDataSource.set = function (queue, requestMethod, directModel, callback) { | ||
queue.add({ | ||
method: requestMethod, | ||
directModel: directModel, | ||
callback: callback | ||
}); | ||
}; | ||
|
||
/** End dataSource **/ | ||
|
||
})(jQuery, fluid_2_0); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
<!DOCTYPE html> | ||
<html lang="en"> | ||
<head> | ||
<meta http-equiv="Content-Type" content="text/html; charset=utf-8" /> | ||
<title>DataSource Tests</title> | ||
|
||
<!-- This is the jqUnit test css file --> | ||
<link rel="stylesheet" media="screen" href="../../../lib/qunit/css/qunit.css" /> | ||
|
||
<script type="text/javascript" src="../../../../src/lib/jquery/core/js/jquery.js"></script> | ||
<script type="text/javascript" src="../../../../src/framework/core/js/Fluid.js"></script> | ||
<script type="text/javascript" src="../../../../src/framework/core/js/FluidIoC.js"></script> | ||
<script type="text/javascript" src="../../../../src/framework/core/js/DataBinding.js"></script> | ||
<script type="text/javascript" src="../../../../src/framework/core/js/FluidRequests.js"></script> | ||
|
||
<!-- These are the jqUnit test js files --> | ||
<script type="text/javascript" src="../../../lib/qunit/js/qunit.js"></script> | ||
<script type="text/javascript" src="../../../test-core/jqUnit/js/jqUnit.js"></script> | ||
|
||
<!-- These are tests that have been written using this page as data --> | ||
<script type="text/javascript" src="../js/DataSourceTests.js"></script> | ||
|
||
</head> | ||
<body> | ||
<h1 id="qunit-header">DataSource Test Suite</h1> | ||
<h2 id="qunit-banner"></h2> | ||
<div id="qunit-testrunner-toolbar"></div> | ||
<h2 id="qunit-userAgent"></h2> | ||
<ol id="qunit-tests"></ol> | ||
|
||
</body> | ||
</html> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May as well just call this "fluid.dataSource" - since we need some form of grade that allows identification of dataSources in a component tree by type. As it stands "emptyDataSource" doesn't have a use except as a placeholder