redis-oplog icon indicating copy to clipboard operation
redis-oplog copied to clipboard

Support addedAt/changedAt/removedAt/movedAt

Open veered opened this issue 7 years ago • 7 comments

Redis oplog doesn't seem to support the addedAt, changedAt, removedAt, or movedAt observe callbacks.

These callbacks are described in the Meteor docs https://docs.meteor.com/api/collections.html#Mongo-Cursor-observe

This could break many use cases, but in particular it breaks Easy Search: https://github.com/matteodem/meteor-easy-search/blob/master/packages/easysearch:core/lib/core/search-collection.js#L217

This can be replicated by running the following code on the server from the Meteor shell:

Meteor.users.find({}).observe({ addedAt(){ console.log('zzz'); } });

Note that zzz is not printed to the logs when redis oplog is enabled.

Edit: Looks like a workaround is falling back to the polling observe driver for observes that require these callbacks, as described in https://github.com/cult-of-coders/redis-oplog/blob/master/docs/finetuning.md.

veered avatar Aug 13 '18 23:08 veered

The oplog observe driver falls back to the polling observe driver on ordered callbacks. Perhaps redis oplog should just do this as well.

veered avatar Aug 14 '18 00:08 veered

It works with the new branch that is in PR, we're only overriding the observerDriver, hooking directly into the core. I still have some small things to do in there, mostly related to sending optimistic ui events async so it doesn't lock.

theodorDiaconu avatar Aug 17 '18 08:08 theodorDiaconu

Oh nice, this one https://github.com/cult-of-coders/redis-oplog/pull/283 ?

veered avatar Aug 17 '18 17:08 veered

@veered yes. The guys from hive.com are using it successfully for over 2 weeks already and they see great improvements, so I would mark it as stable, but I feel that merging it without solving async optimistic ui pushes is like a step-back. I will do my best to fit it in the next weeks to solve it.

theodorDiaconu avatar Aug 20 '18 06:08 theodorDiaconu

@theodorDiaconu #283 has better performance?

veered avatar Aug 21 '18 22:08 veered

Havent tested performance. I think that thos is how it should have been coded since the beginning.

Regards, Theodor

On 22 Aug 2018, at 1:33 AM, Lucas Hansen [email protected] wrote:

@theodorDiaconu #283 has better performance?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

theodorDiaconu avatar Aug 22 '18 05:08 theodorDiaconu

@veered I believe this is working now. That PR got merged and released. Did you have a chance to try it out?

etyp avatar Jun 01 '19 18:06 etyp