Skip to content

FIXED: MQTT retained message consumer creation#5048

Merged
derekcollison merged 3 commits into
mainfrom
lev-mqtt-fix-consumer-name
Feb 7, 2024
Merged

FIXED: MQTT retained message consumer creation#5048
derekcollison merged 3 commits into
mainfrom
lev-mqtt-fix-consumer-name

Conversation

@levb
Copy link
Copy Markdown
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
Comment thread server/mqtt.go
if err != nil {
return nil, err
}
subj := fmt.Sprintf(JSApiDurableCreateT, cfg.Stream, cfg.Config.Durable)
Copy link
Copy Markdown
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
Copy Markdown
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?

Comment thread server/mqtt.go
Comment thread server/mqtt.go
return ccr, ccr.ToError()
}

func (jsa *mqttJSA) createDurableConsumer(cfg *CreateConsumerRequest) (*JSApiConsumerCreateResponse, error) {
Copy link
Copy Markdown
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?

@levb levb requested a review from derekcollison February 7, 2024 18:08
Copy link
Copy Markdown
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
@derekcollison derekcollison deleted the lev-mqtt-fix-consumer-name branch February 7, 2024 20:13
@kozlovic
Copy link
Copy Markdown
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
Copy Markdown
Member

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

@kozlovic
Copy link
Copy Markdown
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.

3 participants