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

Make adapter call a hook when encountering a change for a record that is not yet loaded. #108

Merged
merged 1 commit into from Mar 19, 2016

Conversation

lukemelia
Copy link
Contributor

Pouch adapter now calls a hook method when encountering a change for a record that is not yet loaded.

  • The hook is no-op by default, to match present behavior and performance profile
  • Added a test demonstrating use of the hook to load the new data into the ember-data store

@lukemelia / @chrislopresto

* If you want to change this, subclass this method, and push the data into the store. e.g.
*
* let store = this.get('store');
* let recordTypeName = this.getRecordTypeName(this.store.modelFor(obj.type));
Copy link
Collaborator

Choose a reason for hiding this comment

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

store.modelFor(obj.type) without this?

…a record that is not yet loaded.

- The hook is no-op by default, to match present behavior and performance profile
- Added a test demonstrating use of the hook to load the new data into the ember-data store
@nolanlawson
Copy link
Member

Hmmm, reading this and trying to understand it (forgive me; I am really weak on EmberData specifics, although I grok Pouch and relational-pouch 😃).

I guess what I'm wondering is why we have this early return in the first place. Doesn't this mean that changes from a remote CouchDB will never be loaded into the Ember store? Why don't we just make your commented code the default behavior?

(Side note: it seems like we have a very robust syncing system between PouchDB and CouchDB, but a rather weak one between PouchDB and the in-memory store. In particular, the fact that this syncing logic isn't taking _conflicts or _revisions into consideration worries me a lot. I'm not sure if anything's wrong, but it just seems like a code smell to me.)

@fsmanuel
Copy link
Collaborator

@nolanlawson here is the discussion: #50 (comment)

@lukemelia
Copy link
Contributor Author

@nolanlawson @fsmanuel yes, my understanding based on the README and the referenced thread was that pushing every new record into the store on arrival was an undesirable for performance reasons. We have a low-volume use case and wanted to make it easier to implement that approach.

@rsutphin
Copy link
Collaborator

@nolanlawson, I know you're busy so here's a summary: the ember-data store is the in-memory cache of records the application has touched during the current session. For records which are already cached, our default change watcher updates the cached copy when they change and removes them from the cache when they are removed from PouchDB. It does not automatically cache new records that arrive because that causes terrible performance during the initial sync with medium-and-up data sizes.

(Re: revisions. The change watcher does not update cached records unless ember-data's state machine believes that the cached record is up to date with PouchDB; see this line. If an in-memory copy has been modified but not saved and simultaneously a sync brings in a new rev of that record, the application's attempt to save it will cause a conflict. The application can then resolve the conflict as it sees fit.

(Similarly for _conflicts: if there are conflicts during replication, it's the responsibility of the app to take care of them in PouchDB directly. When the app resolves them, the change watcher will update any loaded records. If the app ignores _conflicts, I guess that the change watcher would receive changes that reflect the default resolutions, but I haven't tried that myself.)

@rsutphin
Copy link
Collaborator

@lukemelia / @chrislopresto: thanks for this. I have an idea about how to make the adapter act more naturally in this area, but I haven't had a chance to implement/propose it yet. Your change would not interfere with what I'm thinking of and it looks like a good interim step.

@lukemelia
Copy link
Contributor Author

Thanks for the summary @rsutphin. With respect to the discussion of conflict handling. This seems like it could also be initially addressed by calling a hook with the relevant details that has no-op behavior. i.e. but the power in the hands of the end-user while providing conservative defaults.

@nolanlawson
Copy link
Member

@rsutphin Ah okay, that makes a lot more sense then. I look forward to the more complete in-memory mirroring solution (I take it something like this would reproduce exactly the performance problem you describe, for the initial load of a large database?), but for now, if you're +1 on this PR, then I am as well. I'll let you merge it when you're ready. :)

@mattmarcum
Copy link
Contributor

👍 is there anything that needs to be done to get this merged?

@nolanlawson
Copy link
Member

I just wanted to give other contributors time to look at it. I'm +1; I'll merge later today when I get a chance.

nolanlawson added a commit that referenced this pull request Mar 19, 2016
Make adapter call a hook when encountering a change for a record that is not yet loaded.
@nolanlawson nolanlawson merged commit 18e0c86 into pouchdb-community:master Mar 19, 2016
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

Successfully merging this pull request may close these issues.

None yet

5 participants