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.

Sign up for GitHub

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

Jump to bottom

fix: add a warning message on startup if the server name is default #14613

Merged
merged 2 commits into from
Oct 27, 2022
Merged

fix: add a warning message on startup if the server name is default #14613

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d4fbfb8
fix: add a warning message on startup if the server name is default
nic-chen Oct 23, 2022
191fb30
fix: apply review suggestion
nic-chen Oct 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions server/embed/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -765,6 +765,15 @@ func (cfg *Config) Validate() error {
return fmt.Errorf("--experimental-compact-hash-check-time must be >0 (set to %v)", cfg.ExperimentalCompactHashCheckTime)
}

// If `--name` isn't configured, then multiple members may have the same "default" name.
// When adding a new member with the "default" name as well, etcd may regards its peerURL
// as one additional peerURL of the existing member which has the same "default" name,
// because each member can have multiple client or peer URLs.
// Please refer to https://github.com/etcd-io/etcd/issues/13757
if cfg.Name == DefaultName {
ahrtr marked this conversation as resolved.
Show resolved Hide resolved
cfg.logger.Warn("the server is using default name, which might cause error that failed the startup.")
Copy link
Member

Choose a reason for hiding this comment

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

I was planning to add a comment for this, but somehow forgot to do it:(

Please change it to something like below,

return fmt.Errorf("it isn't recommended to use %q name, please set a value for --name. Note that etcd might run into issue when multiple members have the same default name", DefaultName)

Copy link
Member

Choose a reason for hiding this comment

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

Please ignore my previous comment. We can't return an error, because etcd may fail to get started. Let's just update the message,

cfg.logger.Warn("it isn't recommended to use %q name, please set a value for --name. Note that etcd might run into issue when multiple members have the same default name", DefaultName)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I will update it later today or tomorrow

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I will update it later today or tomorrow

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated. #14642
please take a look at your convenience. thanks.

}

return nil
}

Expand Down