Bug 819628 #10981
Bug 819628 #10981
Conversation
|
|
||
| // Number of event groups to commit at a time, | ||
| // event Group collection cannot exceed this size | ||
| this.eventCommitChunkSize = 5; |
There was a problem hiding this comment.
put this on the prototype
|
@ganesh7 General comments- If you need to create a struct (anything with many properties) create a private type and use that instead (the groups are my example here) rather then building the object inline. I reallly want to avoid using flags on the pull events object itself because if we change how and when we stream items (lets say we stream events in parallel) bad things will happen and this all breaks. I would suggesting making two bigger changes- First would be to emit a new event (as in event emitter not calendar event) from the service when all the data for a particular calendar event has been sent. Second would be to group data based on event ids and store any metadata (like if a particular group is done) on the group itself if that is really required. |
| if (event.type === 'missingEvents') { | ||
| this.removeList = data[0]; | ||
| return; | ||
| } |
There was a problem hiding this comment.
nit: gaia uses the following style:
if () {
} else {
}|
@ganesh7 I am confused again- what is incremental about this? Looks like we just aggregate all the data then commit it piece by piece after we allocate all of the information? This is probably better then what we have but I would like something that commits as we receive information so we are aggressively optimizing our memory consumption. |
|
@ganesh7 Oops! I am obvious not paying enough attention I see... ignore the above |
|
I think the format for the event groups should be a little different: var groups = {};
groups[event._id] = new EventGroup();
// then associate data with that event group as event._id's come in.That will let us extend event groups later to support sizing logic based on the real size of the group rather then the number of groups. Also I still the new new event approach makes more sense and is alot less brittle if we decide to move things around. stream.on('eventComplete', function(eventId) {
// here we can put logic like (if we have not committed in X seconds then do one) or add it to the queue to be committed.
}); |
| * Resets event Group Object and its related booleans to prepare | ||
| * for the next set of events and its related components | ||
| */ | ||
| resetEventGroup: function() { |
There was a problem hiding this comment.
similar comment as before- I think this should be on its own type/object rather then in this class.
|
@ganesh7 This still does not address all of my comments and most importantly the event api is not here... I made a few small comments. The changes that where made look okay. |
| */ | ||
| function PullEvents(stream, options) { | ||
| function PullEvents(stream, options, callback) { |
There was a problem hiding this comment.
Why do you need to pass a callback here? The consumer can just call instance.on('complete');
| eventStore.findByIds([eventAndComponentId], function(err, list) { | ||
| assert.length(Object.keys(list), 0); | ||
| assert.ok(!list[eventAndComponentId]); | ||
| if (done) { |
There was a problem hiding this comment.
why do you need to check for done? These tests simply won't work in a sync test (so you always need done);
|
@ganesh7 pretty sure there is a bug that causes caldav_pull_events (on line 344) to explode in the case where there are no events to commit. |
…mits Revert "Merge pull request #10981 from ganesh7/bug_819628"
…mits Revert "Merge pull request #10981 from ganesh7/bug_819628"
Bug 819628 - Use incremental commits to stores when synchronizing calendars.