Closed wildhart closed 1 year ago
Any comment on this @mizzao? I would really like to make mizzao:partitioner
compatible with meteor-collection-hooks
0.9+ because I am also suffering from #203 and in my case the entire user document can be 100s of kBs.
I've been trying to merge PR #206 into my own fork of 0.8.4 to fix #203.
I've discovered that changing this one line in meteor-collection-hooks/update.js
:
if (aspects.before || aspects.after) {
to this:
if (!_.isEmpty(aspects.before) || !_.isEmpty(aspects.after)) {
breaks mizzao:partitoner
- an update
which is only supposed to update users in one partition actually updates ALL users. Looks like this PR might have unintended side-effects...
In the meantime, I have added a line to collection-hooks
.js in v0.8.4:
CollectionHooks.getDocs = function getDocs (collection, selector, options) {
const findOptions = { transform: null, reactive: false,
{fields: {_id: 1}}, // I added this line
}
This works because mizzao:partitioner
doesn't actually need the documents in its before hooks, all it does is modify the selector. And nothing else in my app uses collection-hooks
so this is safe for me.
I uploaded this to production about 12 hours ago. The result are as follows, showing method response time of my most common method over 24 hours:
You can see that this has halved my overall method response time, and will have drastically reduced the bandwidth from the database. There are still unnecessary fetches going on, but this is much better.
Sorry @wildhart, I've been too busy with other stuff to look at this at the moment.
Based on your knowledge of this, where should the fix be? In collection-hooks
or mizzao:partitioner
?
I'm really not sure to be honest. I suspect that partitioner
was using a 'feature' of collection-hooks
which meant that update
operations were also using the find
hooks and so didn't need their own hooks. For some reason I don't understand yet, adding the _.isEmpty(aspects...)
breaks that 'feature' so that the update
isn't limited to the current user's group.
So I don't know whether the 'feature' in collection-hooks
was broken by PR #206 (and may break other people's code outside of partitioner
), in which case collection-hooks
needs to be fixed; or whether partitioner
was making an unsafe assumption about the internal workings of collection-hooks
, and that assumption is no-longer valid, in which case partitioner
would need to be fixed.
I also have other priorities at the moment so I don't have much time to investigate further - I've implemented a temporary hack (as above) which mitigates the problem of fetching an entire document whenever it's updated so for the moment I'm happy. I may have more time to help identify the root cause in the new year...
Please could you expand upon the 0.9.0 release note:
I think I have suffered from this breaking change while upgrading from 0.7.15.
I am using mizzao:partitioner to partition my database for multi-tenancy SaaS. In one of many examples I use the following command to update a timestamp for the company belonging to the user which called a particular method:
With
meteor-collection-hooks@0.7.15
this would update the correct document. But with0.9.0
this updates the first document in the companySettings collection. For some reason the hook in mizzao:partitioner which replaced the{}
selector with{groupId: <user's groupId>}
is no longer functioning.Is that due to this breaking change? If so, please can you explain how I can fix my code or mizzao:partitioner to be compatible with 0.9.0?
mizzao:partitioner doesn't hook the
update
method - it's code includes this comment:Perhaps that statement isn't true anymore?