[Added] MQTT: QoS2 support #4349
Conversation
There was a problem hiding this comment.
Finished my review but there are lots of tests that are failing, so clearly some issues. I also need to check that when we "store" (a QoS1/2) a message, I believe that the delivery to JS consumer is automatically initiated, which I think would break the MQTT workflow with PUBREC/REL/COMP. Also note that once a JS message is stored (and delivered to JS consumer), it may be redelivered automatically by JetStream when no ack is received. Again, not sure if that throws a wrench into the compliance.
|
@kozlovic sorry about all the debugging in |
|
@levb Do you need me to do the final review or are you still working on it? |
|
@kozlovic not yet, I am hoping to have it for you tomorrow. |
Adds QoS2 related tests and 2 uncovered bug fixes to #4349. Still WIP/to come: - test that QoS2 session state (pubrel) gets saved/restored. - figure out a test and a solution for saving/restoring the PI mapping/use state when server is restarted @kozlovic I'd really appreciate suggestions on - (a) more tests to add (or modify for QoS2), especially WRT cleanup, clustering - (b) what to do about `[MQTT-3.10.4-3]`, see comments in `TestMQTTQoS2InflightMsgsDeliveredAfterUnsubscribe`
|
@kozlovic I think I addressed previous comments and this PR is now ready for a final review. I'll make a separate PR to eliminate |
|
@derekcollison can we squash-merge this? There were so many "substantial" (as opposed to merging from dev) commits here that keeping the commit history would hardly make sense. I can squash everything in a single commit myself and force-push once we have all conversations resolved? LMK what is the right thing to do. |
|
@levb What Derek said is that when you want to update your PR's branch with the But for sure, once you get the green light for this PR, I would recommend that you squash everything to a single commit and push-force and let Travis re-run all tests, then if all is OK, you will merge into dev. |
|
@derekcollison rebased/squashed |
|
Will let you two handle review and merge. |
…4440) (Partially?) addresses #4349 (comment) @kozlovic @neilalexander I did not remove the use of `domainTk` in the session subject since it seems to have significance to it; removing it failed `TestMQTTSessionsDifferentDomains` and I did not understand the specifics of the issue enough. Please let me know your thoughts.
- [x] Branch rebased on top of current main (`git pull --rebase origin dev`) - [x] Changes squashed to a single commit (described [here](http://gitready.com/advanced/2009/02/10/squashing-commits-with-rebase.html)) - [ ] Build is green in Travis CI ### Changes proposed in this pull request: Removed `qos2` reference from the MQTT outgoing stream name and subjects. Rationale: As I was reviewing the documentation and thinking how to move forward with MQTT, I am inclined to use the `$MQTT_out` stream and session-specific consumers (as opposed to subscription-specific) for all QOS-related packet delivery and PI persistence. I think I will be able to repurpose it "as is", so proposing the generic name change. Since these (stream, consumer, subscriptions) are newly introduced in #4349 and have not yet been released, this is a safe change.
Took the mqtt files files from f9a2efd
@derekcollison @neilalexander @kozlovic
Summary
Adds MQTT QoS2 support