-
Notifications
You must be signed in to change notification settings - Fork 9.8k
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
fix memberList inconsistent output #11812
Conversation
etcdctl member list
etcdctl member list -w=json --hex | python -m json.tool
|
var buffer bytes.Buffer | ||
var b []byte | ||
buffer.WriteString("{\"header\":{\"cluster_id\":\"") | ||
b = strconv.AppendUint(nil, r.Header.ClusterId, 16) |
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.
can you just redefine ClusterID and other IDs as a JSONHexID type? And define a json marshaller on it? So we do not need to reconstruct a full json encoder for the entire memberlistrepons.
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.
something like https://gist.github.com/nikravi/e4812ed46ad77d8d2aee60faddd205df#file-hexbytearray-go-L17-L37 for the ID type.
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.
thanks.i tried this implementation but memberlist response is a pb srtuct and we have to redefine a repeat struct, it looks not good. do you prefer this implementation?
@tangcong The idea looks fine. But I suggest a better implementation. |
if we redefine ClusterID and other IDs as a JSONHexID type, we have to redefine following pb generated structs. @xiang90
|
yes. but i feel redefining the type is better than writing our own encoder, which is error-prone. alternatively, we can write tests to ensure the hex encoder is always correct. |
@@ -110,6 +112,52 @@ func getMemberList(cx ctlCtx) (etcdserverpb.MemberListResponse, error) { | |||
return resp, nil | |||
} | |||
|
|||
func memberListWithHexTest(cx ctlCtx) { |
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.
yes,i added a test case to verify its correctness. @xiang90
lgtm |
We also encountered issue #9975 #9535, this pr supports to print memberlist in hex format json and does not break backward compatibility.
It seems to be a simple problem, but it is not so elegant to solve. I prefer to implement Marshal MemberListResponse manually, rather than redefining a redundant structure to implement the member id Marshal method. @jingyih @gyuho