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

FIXED: MQTT retained message consumer creation #5048

Merged
merged 3 commits into from Feb 7, 2024

Conversation

levb
Copy link
Contributor

@levb levb commented Feb 7, 2024

  • Cleaned up the retained message consumer name so that it does not cause problems
  • Per @derekcollison's recommendation, use an ephemeral consumer for retained messages

@levb levb requested a review from a team as a code owner February 7, 2024 16:54
}
subj := fmt.Sprintf(JSApiDurableCreateT, cfg.Stream, cfg.Config.Durable)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this durable and one above is not?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I broke out the function into 2 to make it more explicit. Do you prefer to keep it exactly as before, 1 function triggering which API to use based on .Durable set?

server/mqtt.go Show resolved Hide resolved
return ccr, ccr.ToError()
}

func (jsa *mqttJSA) createDurableConsumer(cfg *CreateConsumerRequest) (*JSApiConsumerCreateResponse, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this materially different from createConsumer above?

Copy link
Member

@derekcollison derekcollison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@derekcollison derekcollison merged commit f703123 into main Feb 7, 2024
4 checks passed
@derekcollison derekcollison deleted the lev-mqtt-fix-consumer-name branch February 7, 2024 20:13
@kozlovic
Copy link
Member

kozlovic commented Feb 8, 2024

My 2 cents: I had this comment when I implemented MQTT and used a durable for the retained messages:

// Using ephemeral consumer is too risky because if this server were to be
// disconnected from the rest for few seconds, then the leader would remove
// the consumer, so even after a reconnect, we would no longer receive
// retained messages.

Granted that was at the very early stages of JetStream and things may be totally different now, but is that concern still valid or not? If still valid, then I wonder why switching to an ephemeral...

@derekcollison
Copy link
Member

Its not since we can control the inactivity threshold now, vs before it was fixed and fairly low, IIRC 5s.

@kozlovic
Copy link
Member

kozlovic commented Feb 8, 2024

I see, great!

wallyqs pushed a commit that referenced this pull request Feb 13, 2024
- Cleaned up the retained message consumer name so that it does not
cause problems
- Per @derekcollison's recommendation, use an ephemeral consumer for
retained messages
wallyqs added a commit that referenced this pull request Feb 14, 2024
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

Successfully merging this pull request may close these issues.

None yet

3 participants