-
Notifications
You must be signed in to change notification settings - Fork 9.7k
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
etcdmain: better logging when user forget to set initial flags #3556
Conversation
@@ -146,6 +147,19 @@ func Main() { | |||
plog.Errorf("please check (cURL) the discovery token for more information.") | |||
plog.Errorf("please do not reuse the discovery token and generate a new one to bootstrap the cluster.") | |||
default: | |||
if strings.Contains(err.Error(), "match") && strings.Contains(err.Error(), "--initial-cluster") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this deserve to be a specific error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we have variables in this error. i would say no.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see no better way to handle it. Let us do it in this method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unless when we decide to construct ALL specific errors in etcdmain.
LGTM. defer to @crawford |
I still don't understand that original error. Can we fix that? |
@crawford xiang reinvents the log for the original error to be:
Which part makes you confused? |
@crawford I think the original error logging is clear enough, but did not give you a hint of how to fix it. It simply says the two flags you give to etcd does not match with each other. |
As a user, I'm telling you that the error is not clear enough. |
@crawford What do you expect to see? Or why do not think it is not clear enough after providing you all the hints? |
I would rather us fix the original message instead of adding more details. Maybe something like "no initial cluster or discovery URL provided". |
First, the error message should tell you what the error is, rather than finding the cause of the error. The fact is that the two flags does not match with each other. We can make the wording better or providing details based on other context, but not change the error. That is why we add the details. Second, your suggestion is not good. We should not try to hide an error message by providing one possible cause. There are other cases that can cause the error. Also, can you tell me why adding the details is not good enough? It seems that the detailed message after the error message provides what you want based on your reply. |
Agreed, but I do not understand the error message. I don't need my hand held, I just need to be able to comprehend the message.
You can rephrase the original error so that it gives some hint as to how the user got there in the first place. As it exists, one must understand the etcd codebase in order to interpret this error. That's not good.
Well, this is good to know. You know all of the cases; you come up with a better message.
Because instead of fixing the root cause, you've thrown a band-aid over it. I still cannot parse that original error message. |
OK. Let's try to fix this. I would love to fix root cause. Let me explain the error in a verbose english. You can help to make it shorter. As someone who is very familiar with etcd, I think I am not the best person to shorten the error message. To configure a local etcd, you need to set a flag to advertise its own peer urls to others. So other peers can use it to reach the local member. That is called To configure a static cluster, you need to set a to list the cluster members and their advertise-peer-urls including itself's. This flag is called So you can know that you will repeat the peer-url of the local member twice. One in the
You might think that: you can simply remove the duplication. However, we will not. The reason is that we do want the users to repeat this important cluster-wide setting. We do not want one of them to overwrite the other one and we want each member to have exact same |
With all these explanation, I still cannot think a better error message than simply telling the fact that the
Again, I will not change the error message to explain the causes and you agreed on that. If we cannot come up with a better error message. I would rather keep the explanation here. |
working:
Failure 1 (changing name=foo to name=default):
Failure 2 (changing advertise-peer-urls to be one that is not part of initial-cluster):
|
With that it feels like the right error for "failure 2" above is: "--initial-cluster must include foo=http://localhost:9000" |
@polvi Right. That is another way to explain the mismatch. However, we are not sure if the information provided in For example, when user forget to set etcd --name=foo --initial-cluster='foo=http://localhost:9999' We will print out that |
@crawford does this make sense to you at all?
|
@polvi yeah, that makes much more sense. |
61834a5
to
3b70bf8
Compare
etcdmain: better logging when user forget to set initial flags
Based on that commit it looks like it does not do the right thing. Could you run the code path with multiple advertise-urls and print the resulting message? |
@polvi
You want the error message to suggest the format that you can paste into the flag? |
Fix #3552
Now it will log as if you forgot to set both
--initial-cluster
and--discovery
and the configuration does not match.