moscajs / aedes-persistence-mongodb

MongoDB persistence for Aedes
MIT License
14 stars 16 forks source link

Change TTL indexes to packet.added key #51

Closed schwing closed 4 years ago

schwing commented 4 years ago

Fixes https://github.com/moscajs/aedes-persistence-mongodb/issues/50

robertsLando commented 4 years ago

@schwing I would add a test here too: https://github.com/moscajs/aedes-persistence-mongodb/blob/master/test.js#L517

Just add some packets to other collections and check if them are removed like we are doing with retained collection now

schwing commented 4 years ago

@robertsLando Thanks, I didn't notice those other tests before. I've added tests for all the collections there and also for the index tests for better coverage.

I'm unable to reproduce the latest CI test failures shown for 10.x or 13.x here. Is this possibly a timing issue where ready is sometimes emitted prior to indexes being created because the createIndex() calls don't wait for callbacks here? https://github.com/moscajs/aedes-persistence-mongodb/blob/master/persistence.js#L97-L116

robertsLando commented 4 years ago

Is this possibly a timing issue where ready is sometimes emitted prior to indexes being created because the createIndex() calls don't wait for callbacks here?

It could be the reason, could you try to fix that?

robertsLando commented 4 years ago

@schwing I think it would be better to add callbacks support for create index. YOu could use fastparallel https://www.npmjs.com/package/fastparallel

schwing commented 4 years ago

@robertsLando Agreed. I'll add that.

schwing commented 4 years ago

Added and tests look good now. I attempted to match style with the rest of the module. Let me know any thoughts.

robertsLando commented 4 years ago

@schwing Just released a new patch version with this fix :+1:

schwing commented 4 years ago

Great! Thanks for all the assistance here.