Skip to content
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

Add delivered event #10

Closed
behrad opened this issue Nov 19, 2015 · 16 comments
Closed

Add delivered event #10

behrad opened this issue Nov 19, 2015 · 16 comments

Comments

@behrad
Copy link
Collaborator

behrad commented Nov 19, 2015

delivered event is not added to aedes yet.

@mcollina
Copy link
Collaborator

Yes, it's missing. It should be easy to add though.

Just for a reference, here is where it is originally emitted in Mosca:
https://github.com/mcollina/mosca/blob/2694214e230a6b1febfd40fa55c7d33d40876fb0/lib/client.js#L383

@behrad
Copy link
Collaborator Author

behrad commented Nov 19, 2015

yea, and that line was committed by me if you remember @mcollina :)) same for this topic ;)

I want to make sure the very first version of aedes is feasible for me to port my app on, this is my main concern 👍

@guptaashishiitr
Copy link

@mcollina : Do you know when can we have delivered event?

@behrad
Copy link
Collaborator Author

behrad commented Jan 10, 2016

I'll create a PR this week @guptaashishiitr

@pkinney
Copy link
Contributor

pkinney commented Mar 16, 2016

Submitted a PR #39 that should address this. Let me know if this is the desired behavior. I tried to repeat what was done in Mosca referenced above.

@behrad
Copy link
Collaborator Author

behrad commented Apr 16, 2016

Current ackevent is passing the puback packet instead of the original packet delivered. Mosca is passing the original packet, so that application can use topic/payload to know which published message the ack is associated to.

Another nice solution could be the packet.messageId however application is un-aware of that id in publish event listener, in the latter case Aedes should also pass broker's messageId back to the publish listener.

One of the above should be implemented @mcollina

@mcollina
Copy link
Collaborator

@behrad you are right. This change requires a change in the persistences modules, mainly the call to https://github.com/mcollina/aedes-persistence#instanceoutgoingclearmessageidclient-packet-callbackerr need to also return the packet.

Moreover, for QoS 2 we need to change https://github.com/mcollina/aedes-persistence#instanceoutgoingupdateclient-packet-callbackerr to retain the original packet (currently it is overwritten during the QoS 2 flow), or at least keep topic and payload around.

@mcollina
Copy link
Collaborator

mcollina commented Jun 16, 2016

@behard Can you give some help in this one? It will probably require a bit of rework on the persistences.

@behrad
Copy link
Collaborator Author

behrad commented Jun 16, 2016

Sure, I'll check this.

@behrad
Copy link
Collaborator Author

behrad commented Jun 17, 2016

Moreover, for QoS 2 we need to change https://github.com/mcollina/aedes-persistence#instanceoutgoingupdateclient-packet-callbackerr to retain the original packet (currently it is overwritten during the QoS 2 flow), or at least keep topic and payload around.

for QOS 2 shouldn't we ack on PUBCOMP instead?

@behrad
Copy link
Collaborator Author

behrad commented Jun 17, 2016

I don't see how emit('ack') is called for QOS 2 now in aedes!? @mcollina

@behrad
Copy link
Collaborator Author

behrad commented Jun 17, 2016

Please first merge moscajs/aedes-persistence#2
After updating aedes-persistence inside, #54 will pass :)

@mcollina
Copy link
Collaborator

@behrad

I don't see how emit('ack') is called for QOS 2 now in aedes!?

Both pubcomp and puback are handled in the same way.
https://github.com/mcollina/aedes/blob/fd95dfc065243d150285f477b6adcd0aff10dbb1/lib/handlers/index.js#L35-L38

@behrad
Copy link
Collaborator Author

behrad commented Jun 17, 2016

Ok great, I was looking for a pubcomp.js file in handlers ;)

@behrad
Copy link
Collaborator Author

behrad commented Jun 17, 2016

I also filed moscajs/aedes-persistence-redis#5, do you think these 3 PRs can now be merged @mcollina ?

@mcollina
Copy link
Collaborator

Releasing now (aedes is 0.19.0).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants