search

moquette-io / moquette

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

Fix 653 retained messages publishing #658

Closed hylkevds closed 2 years ago

hylkevds commented 2 years ago

This PR adds a test for, and fixes #653.

Retained messages used a difference code-path that skipped the Session, and thus did not properly set up in-flight status. To fix this, this PR:

hylkevds commented 2 years ago

@andsel: Ready for review. The build issue is fixed, on the build-server the initial retained publish was processed slow enough for the subscription to finish first, causing the initial publish to be sent to the subscriber, and not the retained message. The test is more robust now.

andsel commented 2 years ago

@hylkevds another quick note, these tests seems more suitable to be parameterized test: https://junit.org/junit5/docs/current/user-guide/#writing-tests-parameterized-tests

hylkevds commented 2 years ago

@hylkevds another quick note, these tests seems more suitable to be parameterized test: https://junit.org/junit5/docs/current/user-guide/#writing-tests-parameterized-tests

Yep, I copied the test from ServerIntegrationQoSValidationTest, we should convert that one too at some point.

andsel commented 2 years ago

Was stolen from cpp practice, m stay for member, it dates when Moquette was started and the same practice was used by Apache Felix, so I stool the idea. But the time proven, it's useless, also the 'this.' with ide like idea are just more text to read.

Il dom 20 feb 2022, 19:58 Hylke van der Schaaf @.***> ha scritto:

@.**** commented on this pull request.

In broker/src/test/java/io/moquette/integration/ServerIntegrationRetainTest.java https://github.com/moquette-io/moquette/pull/658#discussion_r810669265:

  • private static IConfig m_config;
  • private static Server m_server;

No problem. What was the reason you used that prefix originally?

— Reply to this email directly, view it on GitHub https://github.com/moquette-io/moquette/pull/658#discussion_r810669265, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAH5RUPHBVOXQRGYUEUYIBDU4E2U3ANCNFSM5O3PERJA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you were assigned.Message ID: @.***>

hylkevds commented 2 years ago

Hi @hylkevds, left just a couple of notes about the naming (not vs non)

Is that a convention you use? Since nonretained is the proper English variant. the sentence

send (a) not retained publish

is not correct English. While the sentence

send (a) nonretained publish

is correct... Hence my automatic use of the "non" variant. Even the MQTT standard uses non-retained...

Since there are other instances of not- i've committed your suggestions. It's better to do language changes like that, if we decide to make them, in a separate PR.

andsel commented 2 years ago

@hylkevds about https://github.com/moquette-io/moquette/pull/658#issuecomment-1050650027 my bad, keep the non