-
-
Notifications
You must be signed in to change notification settings - Fork 23
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
feat!: clusters improvements #109
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
).then((keys) => { | ||
// keys: [['key1', 'key2'], ['key3', 'key4']] | ||
// flatten the array | ||
cb(null, keys.reduce((acc, val) => acc.concat(val), [])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wrap this in a nexttick or the callback might be called twice in case of errors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you explain me why this could be called twice? In case one of the promises fails the catch
should be thrown, also why should nextick fix that problem?
Did some preliminary testing on this and seems to work 👍 |
Thanks for the feedback @alumowa let me know when it's good to merge 👍🏻 |
@robertsLando feel free to merge it. tks!!! |
Done 🙏🏼 |
Fixes #85
BREAKING CHANGE: For users using this module with CLUTERS the db schema has changed for
retained
messages. There is a migration available inmigrations.js
namedfrom9to10
that can be used to move all db to the new format. Also, if you are passing aconn
to options (that is using clusters) now you also have to setcluster : true
in options. To make this change back compatibile and faster in non cluster envs that option is used in order to use a different set of operations that only works in cluster envs