These are chat archives for deployd/contributors

29th
Jan 2015
Andrei Alecu
@andreialecu
Jan 29 2015 12:02
actually you might need to check if fn is empty before calling it in internal client, because now it won't be anything
regarding: dpd.collection.del({id:’123'}) if you did this in current master the callback counter will desync, there won't be one added but one will be substracted
so it will be -1
so if you have other calls in there that you expect to wait for, it will affect those and one of them won't be waited fr
for
Andrei Alecu
@andreialecu
Jan 29 2015 12:42
@0ff, I found some bugs, I'll send you another PR
Andrei Alecu
@andreialecu
Jan 29 2015 13:49
PR sent
.catch() was completely broken before this in several ways
I improved the tests a bit to verify most of what I thought about
Andrei Alecu
@andreialecu
Jan 29 2015 14:41
deployd/deployd#500 please merge this when you can, fixes some random test runs I kept getting
test run failures*
Nicolas Ritouet
@NicolasRitouet
Jan 29 2015 15:13
huge PR, it will take weeks for me to review that :D
rebased
@ericfong
Andrei Alecu
@andreialecu
Jan 29 2015 16:20
new PR with a fix: deployd/deployd#501, not terribly complicated
Andrei Alecu
@andreialecu
Jan 29 2015 16:42
I commented on this: deployd/deployd#488
but I just realized something
deployd will let you do that but it won't call the DELETE event for every row
Andrei Alecu
@andreialecu
Jan 29 2015 19:07
I think we should disable this because the fact it actually deletes more than one item looks like a bug
but if you do, we’d need to implement bulk delete, don’t we?
at least make it take a list of ids?
yes, I know, but then you force ppl into running .del in a loop, which is pretty bad
Andrei Alecu
@andreialecu
Jan 29 2015 19:09
the problem is that the delete event has a this consisting of an array of records
when you try to trick it with the $ne
I see
Andrei Alecu
@andreialecu
Jan 29 2015 19:10
so if you have a big collection, that would unnecessarily grab the whole collection in there
what I would suggest is, a separate 'bulk delete' event
hm
Andrei Alecu
@andreialecu
Jan 29 2015 19:11
when a query is detected that would return more than one row, that event is executed instead, and instead of this being prefilled with records, you just get a this.query instead and you can use the internal client if you want to see what it matches
but we do have the result already, don’t we? why not just pass it to the delete event?
Andrei Alecu
@andreialecu
Jan 29 2015 19:12
we only have the result now because the .remove() method is broken here:
or do it like in get - run the event for each and every result?
that should've been store.first
and ideally it should've checked if query.id is a string as well, in the check above
we could run the event for each and every result yeah, but my concern was that if you have a big table and you match a lot of records, they will all be unnecessarily retrieved
and then iterated
hm, the doc for this function is inconsistent in it’s own: Execute adeleteevent script, if one exists, using each object found. Then remove a single object that matches thectx.query.id``
Andrei Alecu
@andreialecu
Jan 29 2015 19:14
yeah the documentation is notoriously bad everywhere in the code actually :)
also the problem with iteration is that once you start iterating, the only way to propagate the delete to mongo afterwards is to delete every single record by id
well but that’s just the way it is then. Ideally events should support multiple items, instead of being called multiple times - because then the event itself can deal with it
Andrei Alecu
@andreialecu
Jan 29 2015 19:16
so 1000 rows will mean 1000 separate deletes by id instead of just one delete query
you mean because of cancel?
Andrei Alecu
@andreialecu
Jan 29 2015 19:16
yes
I disagree on this - just take the query to find, call event(s), call 1 mongo-delete with a query “… id in [list of not cancelled ids]"
Andrei Alecu
@andreialecu
Jan 29 2015 19:20
could do that.. still inefficient
maybe a feature in the future for bulk events
opt-in per collection
bulkget/bulkdelete
called once instead of in iteration
nice idea, I like it a lot actually
could simply be made into an extension
Andrei Alecu
@andreialecu
Jan 29 2015 19:22
and the problem with iterating this way is that this deployd/deployd#496 needs to be merged for deletes to not destroy the server when you have 1000+ rows
yup, but this should be merged anyway :)
Andrei Alecu
@andreialecu
Jan 29 2015 20:20
deployd/deployd#502
well, I fixed it anyway, this will call the Delete event for every record now, should there be a bulk delete involved
Nicolas Ritouet
@NicolasRitouet
Jan 29 2015 20:25
Hey @/all I’m on vacation next week, I won’t be able to rebase and review the PR
but when I’m back, I plan to work on 0.8, finally release
I’d like to freeze the PR that will go into 0.8, so if you don’t think your PR needs to be in 0.8, please, add this in the comment
Andrei Alecu
@andreialecu
Jan 29 2015 20:27
this one is probably important because it's like a SQL injection attack more or less
Nicolas Ritouet
@NicolasRitouet
Jan 29 2015 20:27
can you add this in the comment? I won’t remember it
Andrei Alecu
@andreialecu
Jan 29 2015 20:35
added