search

moquette-io / moquette

Java MQTT lightweight broker
http://moquette-io.github.io/moquette/
Apache License 2.0
2.31k stars 818 forks source link

Add Ping Request handling to the interceptors #481

Open djbr-david opened 5 years ago

djbr-david commented 5 years ago

Expected behavior

Ping Request messages should be notified using the Interceptor interface. This is useful for monitoring client status.

Actual behavior

Currently Ping Requests are handled without any notification

Steps to reproduce

Feature request

Minimal yet complete reproducer code (or URL to code) or complete log file

N/A

Moquette MQTT version

0.12.1

JVM version (e.g. java -version)

Any

OS version (e.g. uname -a)

Any

andsel commented 5 years ago

Hi, why ping requests should be notified with interceptors, which is the use case for an application that embed Moquette to know each ping from every connected client?

Il mer 5 giu 2019, 13:23 David Brooks notifications@github.com ha scritto:

Expected behavior

Ping Request messages should be notified using the Interceptor interface Actual behavior

Currently Ping Requests are handled without any notification Steps to reproduce

Feature request Minimal yet complete reproducer code (or URL to code) or complete log file

N/A Moquette MQTT version

0.12.1 JVM version (e.g. java -version)

Any OS version (e.g. uname -a)

Any

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/moquette-io/moquette/issues/481?email_source=notifications&email_token=AAH5RUNUSONGCDO5AHH4MTDPY6O4JA5CNFSM4HTVVFCKYY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4GXX2EOQ, or mute the thread https://github.com/notifications/unsubscribe-auth/AAH5RUPXSIN4F4QXRY66WUTPY6O4JANCNFSM4HTVVFCA .

djbr-david commented 5 years ago

Hi. The use case is purely from an Application Support view. Operationally these messages don’t do anything, but support teams like to see this sort of thing as confirmation that the client is still connected, still sending data and isn’t in some zombie state. In our app we have a status page for the broker showing the latest messages received and knowing pingreq is still happening is a useful feature for the support team

andsel commented 5 years ago

Hi @djbr-david I understand you requirements, we could have 2 solutions, in my opinion:

Personally i prefer the second one, because the broker doesn't flood too much notification to the interceptors. Would you like to provide an PR with this? In case remember to cover it with unit tests

djbr-david commented 5 years ago

Hi - apologies for not replying earlier, I didn't see your second message until just now. I'll take a look at adding both options to the project to provide the most flexibility.

djbr-david commented 5 years ago

Thinking of option 2 again, isn't that already covered by the use of the onDisconnect and onConnectionLost interceptors?

andsel commented 5 years ago

Yes, you are right! good catch

On Tue, Jul 2, 2019 at 4:58 PM David Brooks notifications@github.com wrote:

Thinking of option 2 again, isn't that already covered by the use of the onDisconnect and onConnectionLost interceptors?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/moquette-io/moquette/issues/481?email_source=notifications&email_token=AAH5RULVP6SLZ3GDQVXXBT3P5NUINA5CNFSM4HTVVFCKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZBR3QI#issuecomment-507715009, or mute the thread https://github.com/notifications/unsubscribe-auth/AAH5RULH77JBO23CXOPAHULP5NUINANCNFSM4HTVVFCA .