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

Feature Request: Different notification for Bulk Update complete #123

Open
cognitiveflux opened this issue Oct 31, 2014 · 5 comments
Open

Comments

@cognitiveflux
Copy link
Contributor

There doesn't appear to be a hook or notification if using the bulk_update {} block. Perhaps there's a recommended way to do this that's not documented? Otherwise, I'm willing to give the implementation a shot if you define any restrictions or caveats for implementation.

I'm proposing something like a MotionModelDataDidCompleteBulkUpdateNotification, thoughts?

@sxross
Copy link
Owner

sxross commented Oct 31, 2014

This would be extremely helpful. I've found that dribbling out updates in groups of 10 or something is a very effective way to keep the UI alive. Consider:

bulk_update(notify_interval: 10) {
  # some stuff
}

That way, you could issue MotionModelProgressNotification and at the end MotionModelDataDidCompleteBulkUpdateNotification. WDYT?

@cognitiveflux
Copy link
Contributor Author

I've noticed that record sets of 100 load incredibly quick, but I agree and think it's best to allow the frequency to be defined as in your example so people can fine tune to their needs and data types. I'll familiarize myself with how MotionModel handles notifications.

A couple requirement specs/clarifications:

  • If the notify_interval is not provided, default to only sending the completion?
  • What is the significance of 'completion': does it mean that the update was performed but perhaps not all records were created? Or will MotionModel raise or notify if an individual create fails? Are there any guarantees of complete success or complete failure?

@sxross
Copy link
Owner

sxross commented Oct 31, 2014

I would default to completion (i.e., end of block) if interval is not set. The latency is typically not in MotionModel, but in remote services like Firebase or (in the case of my current app) Meteor. They try to do as-available data supply, which can be chunked. I've found that the app "looks" like it's hanging on startup if I don't show something every so often.

When an interval is provided, completion is defined as "all the rest are updated". This would only be relevant if the supplier sent a "ready" notification, but it still useful. I think this deserves some thought, as timeout might be another consideration for data suppliers that throttle.

@cognitiveflux
Copy link
Contributor Author

Are you thinking of usage like the following?:

async_remote_request do |response|
   bulk_update do
     response.body.each { |r| Model.create(r) }
   end
end

If you're doing the bulk update after the response is received the timeout wouldn't be a concern because each chunck will perform a bulk_update, correct?

With the new notifications, the UI can be updated each time the chunk is received, instead of when each record is created (currently can't use bulk_update since no notification is fired, so using a normal create for every record fires a notification). Using the above example, I'm not sure what benefit the notify_interval option would provide.

If you were thinking of wrapping the remote request within the bulk_update block then there are a myriad of concerns that come up and it might be best to not mix the task of handling remote data sources within the context of MotionModel handling local data. Thoughts?

@sxross
Copy link
Owner

sxross commented Nov 1, 2014

I misspoke. Of course you're right about not wrapping request/response inside one of these blocks. The request/response would (or should) run asynchronously, and the block would almost immediately complete, triggering the completion notification. The use case for :notify_interval I was referring to was where object creation was complicated and possibly time-consuming. Perhaps "interval" is poor semantics. Maybe something like:

# Option 1: Add lots of syntactic sugar
bulk_update(and_progress_notify: every(10).items) {
  # some stuff
}

# Option 2: More terse, but still shows intent
bulk_update(notification_item_frequency: 10) {
  # some stuff
}

I don't want to overthink this. But I do think completion notification will be used. I know I have already done both that and intervals, but not backported them to MotionModel (my bad).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants