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

[CHANGED] MQTT no longer requires server name to be set #4679

Merged
merged 4 commits into from
Oct 25, 2023

Conversation

levb
Copy link
Contributor

@levb levb commented Oct 19, 2023

It was required because it was required by JetStream, but JetStream now auto-generates a default name.

@levb levb requested a review from bruth October 19, 2023 07:04
@levb levb requested a review from a team as a code owner October 19, 2023 07:04
Copy link
Member

@bruth bruth left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@@ -598,11 +597,6 @@ func validateMQTTOptions(o *Options) error {
if mo.Port == 0 {
return nil
}
// We have to force the server name to be explicitly set. There are conditions
// where we need a unique, repeatable name.
if o.ServerName == _EMPTY_ {
Copy link
Member

@derekcollison derekcollison Oct 19, 2023

Choose a reason for hiding this comment

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

This might be ok for single server mode, but not clustered mode. In clustered mode this is required.

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 thought @bruth's point was that a (unique?) name will be auto-defaulted, not? I don't see any direct references to it in the MQTT code.

Copy link
Member

Choose a reason for hiding this comment

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

Peer names, which need to be consistent, are a derived hash from server names. So in clustered mode this condition has to hold. So we need to check if we are in single server mode, or not using JetStream, although I thought it was required for MQTT.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

JS is required for MQTT; I am 0/5 if it makes any sense to remove the name requirement for single server only. @bruth?

Copy link
Member

Choose a reason for hiding this comment

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

There is a js helper function to test if in single server mode.

Copy link
Member

Choose a reason for hiding this comment

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

Let's do same thing.

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 did, unfortunately it just checks that the run-time object is not nil, that it has initialized. This one, https://github.com/nats-io/nats-server/blob/main/server/jetstream_cluster.go#L724?

Copy link
Member

Choose a reason for hiding this comment

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

// Determines if this server is in standalone mode, meaning no routes or gateways.
func (s *Server) standAloneMode() bool {
	opts := s.getOpts()
	return opts.Cluster.Port == 0 && opts.Gateway.Port == 0
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah! thx!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@derekcollison Can you please confirm

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 9d3208b into main Oct 25, 2023
4 checks passed
@derekcollison derekcollison deleted the lev-do-not-require-server-name branch October 25, 2023 15:26
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