[FIXED] MQTT fixes and improvements #2178
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kozlovic
commented
May 4, 2021
matthiashanel
approved these changes
May 4, 2021
Some issues that have been fixed would manifest by timeouts on connect, unexpected memory usage on high publish message rate. Some details: - Replies were not always GW routed properly because we were looking at the wrong connection's rsubs - GW routed replies would not be found because they were tracked in the subscription's client object, which may not be the same used to send the reply - Increased the mqtt timeout to wait for JS replies since in some tests it was sometimes taking more than the original 2 seconds - Incoming gateway messages destined for an MQTT internal subscription may have been rejected as a no interest if the account had service imports - Don't use time.After(), instead create explicit timer so it can be stopped when not timing out. - Unnecessary copy of a slice since we were converting to a string anyway. Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
kozlovic
changed the title
|
@derekcollison I have been reworking all that quite a bit and rebased from main, sorry about that. The main change has been to keep the gw reply mapping in the client object for CLIENT connections, and to account for others. I have extracted the client's gwrm/cgwrt to its own struct so that code can be reused between client/account, etc.. |
This will solve the issue of naming the durable per server for
the "retained messages" stream in situation where a cluster
of servers would not have JetStream defined but connect to another
cluster that has it. All the servers within the cluster without
JetStream would cause the durable's delivery subject to be updated
to the last server starting the durable.
Updated the check for mqtt requiring JetStream if running in
standalone mode to check that no leafnode configuration is present.
Replaced use of fmt.Errorf() when the string was static with
errors created with errors.New(). Updated tests that were checking
for errors to use those errors instead of repeating the string.
Added test that has a hub cluster with JS enabled and a remote server
that has mqtt{} without JetStream and ensure that this works.
Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Some issues that have been fixed would manifest by timeouts on
connect, unexpected memory usage on high publish message rate.
Some details:
at the wrong connection's rsubs
in the subscription's client object, which may not be the same used
to send the reply
tests it was sometimes taking more than the original 2 seconds
may have been rejected as a no interest if the account had service imports
be stopped when not timing out.
Signed-off-by: Ivan Kozlovic ivan@synadia.com