Print clusterID, memberID and leaseID in hexdecimal #14208

ahrtr · 2022-07-11T07:23:04Z

ClusterID, memberID and leaseID are just identities, so it doesn't make sense to print them in decimal. Instead, we should always print them in hexadecimal.

Please note the existing behavior isn't consistent. Sometimes etcdctl prints them in hexdecimal (such as etcdctl endpoint status -w table), but sometimes in decimal (such as etctctl endpoint status -w fields).

Previous output

wachao-a01:bin wachao$ ./etcdctl  member list -w fields
"ClusterID" : 14841639068965178418
"MemberID" : 10276657743932975437
"RaftTerm" : 5
"ID" : 10276657743932975437
"Name" : "default"
"PeerURL" : "http://localhost:2380"
"ClientURL" : "http://localhost:2379"
"IsLearner" : false

wachao-a01:bin wachao$ ./etcdctl endpoint status -w fields
"ClusterID" : 14841639068965178418
"MemberID" : 10276657743932975437
"Revision" : 3
"RaftTerm" : 5
"Version" : "3.6.0-alpha.0"
"StorageVersion" : "3.6.0"
"DBSize" : 24576
"DBSizeInUse" : 24576
"Leader" : 10276657743932975437
"IsLearner" : false
"RaftIndex" : 23
"RaftTerm" : 5
"RaftAppliedIndex" : 23
"Errors" : []
"Endpoint" : "127.0.0.1:2379"

wachao-a01:bin wachao$ ./etcdctl  lease list -w fields
"ClusterID" : 14841639068965178418
"MemberID" : 10276657743932975437
"Revision" : 3
"RaftTerm" : 8
"ID" : 7587863798256828424
"ID" : 7587863798360740612
"ID" : 7587863798360740614
"ID" : 7587863798256828422

New output

wachao-a01:bin wachao$ ./etcdctl member list -w fields
"ClusterID" : cdf818194e3a8c32
"MemberID" : 8e9e05c52164694d
"RaftTerm" : 7
"ID" : 8e9e05c52164694d
"Name" : "default"
"PeerURL" : "http://localhost:2380"
"ClientURL" : "http://localhost:2379"
"IsLearner" : false

wachao-a01:bin wachao$ ./etcdctl endpoint status -w fields
"ClusterID" : cdf818194e3a8c32
"MemberID" : 8e9e05c52164694d
"Revision" : 3
"RaftTerm" : 7
"Version" : "3.6.0-alpha.0"
"StorageVersion" : "3.6.0"
"DBSize" : 24576
"DBSizeInUse" : 24576
"Leader" : 10276657743932975437
"IsLearner" : false
"RaftIndex" : 27
"RaftTerm" : 7
"RaftAppliedIndex" : 27
"Errors" : []
"Endpoint" : "127.0.0.1:2379"

wachao-a01:bin wachao$ ./etcdctl  lease list -w fields
"ClusterID" : cdf818194e3a8c32
"MemberID" : 8e9e05c52164694d
"Revision" : 3
"RaftTerm" : 7
"ID" : 694d81ec1b250008
"ID" : 694d81ec1b250006

Signed-off-by: Benjamin Wang wachao@vmware.com

ahrtr · 2022-07-11T07:26:11Z

cc @serathius @ptabor @spzala

@liggitt @dims @neolit123 do you have any concerns from K8s perspective?

codecov-commenter · 2022-07-11T07:54:15Z

Codecov Report

Merging #14208 (5cae1a1) into main (20bf49c) will decrease coverage by 0.40%.
The diff coverage is 8.57%.

@@            Coverage Diff             @@
##             main   #14208      +/-   ##
==========================================
- Coverage   75.49%   75.09%   -0.41%     
==========================================
  Files         455      455              
  Lines       36859    36908      +49     
==========================================
- Hits        27828    27715     -113     
- Misses       7299     7441     +142     
- Partials     1732     1752      +20

Flag	Coverage Δ
all	`75.09% <8.57%> (-0.41%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
etcdctl/ctlv3/command/printer_fields.go	`6.09% <5.88%> (-1.10%)`	⬇️
etcdctl/ctlv3/command/printer.go	`23.20% <100.00%> (ø)`
server/etcdserver/api/rafthttp/peer_status.go	`87.87% <0.00%> (-12.13%)`	⬇️
server/auth/simple_token.go	`80.00% <0.00%> (-8.47%)`	⬇️
server/etcdserver/api/rafthttp/peer.go	`87.01% <0.00%> (-8.45%)`	⬇️
client/pkg/v3/fileutil/lock_linux.go	`72.22% <0.00%> (-8.34%)`	⬇️
client/pkg/v3/tlsutil/tlsutil.go	`83.33% <0.00%> (-8.34%)`	⬇️
client/v3/leasing/util.go	`91.66% <0.00%> (-6.67%)`	⬇️
server/etcdserver/api/v3rpc/util.go	`74.19% <0.00%> (-6.46%)`	⬇️
client/v3/concurrency/mutex.go	`61.64% <0.00%> (-5.48%)`	⬇️
... and 31 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 20bf49c...5cae1a1. Read the comment docs.

neolit123 · 2022-07-11T10:30:24Z

Given the discrepancy is exposed to the user on the CLI it might make sense to make it possible for them opt out with a flag for a transitional period until the the behavior is consistent accross the CLI commands.

In terms of hex vs decimal for IDs - it depends. Decimal makes it easier to process / human parse IDs in e.g. scripts.

ptabor

I agree it would be better to introduce this in more backward compatible way.
The fastest path I see:

Introduce flag in 3.6 defaulting to 'hex'
Backport the PR to 3.5, but change the flag there to decimal. In 3.5, when the flag is unset, there should be a warning about behavioral change in 3.6
Remove the flag in 3.7 (or 4).

ptabor · 2022-07-13T14:13:31Z

etcdctl/ctlv3/command/printer_fields.go

 }

 func (p *fieldsPrinter) hdr(h *pb.ResponseHeader) {
-	fmt.Println(`"ClusterID" :`, h.ClusterId)


Can we cast it to types.Id to benefit from the common stringer implementation:

(

etcd/client/pkg/types/id.go

Lines 22 to 24 in 53bfdc1

type ID uint64

func (i ID) String() string {

) ?

Thanks @ptabor . It makes sense. But only memberID and clusterID are uint64, and LeaseID is int64. So I only cast memberID and clusterID to types.ID.

ptabor · 2022-07-13T14:19:58Z

In terms of hex vs decimal for IDs - it depends. Decimal makes it easier to process / human parse IDs in e.g. scripts.

Storing this as typed integer saves a few bytes but caries risk of `signed vs. unsigned' mismatch and overflow. As we don't compare this values for inequality I think that hex are steering users towards safer code where they would script them as 'strings'.

ClusterID, memberID and leaseID are just identifies, so it doesn't make sense to print them in decimal. Instead, we should always print them in hexdecimal. Signed-off-by: Benjamin Wang <wachao@vmware.com>

ahrtr · 2022-07-15T23:39:41Z

Thanks @neolit123 and @ptabor for the feedback, which basically makes sense to me.

Actually I thought about backward compatible, the reason I did not do it was that there is no any change on JSON output which should be the most common case for programmatically parsing the output.

But anyway, I updated this PR and leverage the existing flag --hex

wachao-a01:bin wachao$ ./etcdctl  member list -w fields --hex
"ClusterID" : cdf818194e3a8c32
"MemberID" : 8e9e05c52164694d
"RaftTerm" : 2
"ID" : 8e9e05c52164694d
"Name" : "default"
"PeerURL" : "http://localhost:2380"
"ClientURL" : "http://localhost:2379"
"IsLearner" : false


wachao-a01:bin wachao$ ./etcdctl  lease list -w fields --hex
"ClusterID" : cdf818194e3a8c32
"MemberID" : 8e9e05c52164694d
"Revision" : 1
"RaftTerm" : 2
"ID" : 694d820437ab0a07

PTAL @ptabor @neolit123 @serathius @spzala thx

spzala

@ahrtr extending the current flag that's used for json seems good approach! I have tested it my env to verify. I think we should add Changelog and also how about adding a test? It's okay to add it in a separate PR if you want. Thanks!

ahrtr · 2022-08-11T22:06:37Z

Thanks @spzala for the review comment, which basically makes sense to me.

Points from my side:

Unfortunately, there is no any unit tests for any command so far. So I will investigate how to add unit test and add an example case firstly in a separate PR; afterwards, let's get more contributors involved to add unit tests for all commands. What do you think?
With regard the Changelog, I intentionally did not do it, because there are some other related changes (such as server: Implement compaction hash checking #14120 (comment) , and the JSON output is also printing decimal for MemberID etc.) to be done. So I plan to add the ChangeLog after all the related items are done.

spzala · 2022-08-12T13:35:27Z

Thanks @spzala for the review comment, which basically makes sense to me.

Points from my side:

1. Unfortunately, there is no any unit tests for any command so far. So I will investigate how to add unit test and add an example case firstly in a separate PR; afterwards, let's get more contributors involved to add unit tests for all commands. What do you think?

2. With regard the Changelog, I intentionally did not do it, because there are some other related changes (such as [server: Implement compaction hash checking #14120 (comment)](https://github.com/etcd-io/etcd/pull/14120#issuecomment-1195159939) , and the JSON output is also printing decimal for MemberID etc.) to be done. So I plan to add the ChangeLog after all the related items are done.

@ahrtr thank you, and both the points you mentioned sounds good to me. I will be happy to share work on adding unit test coverage.

spzala · 2022-08-12T13:38:58Z

@ahrtr considering @ptabor is away for sometime and his comments are addressed, we should be good with merging changes, if that sounds good to you?. Thanks!

ahrtr · 2022-08-12T21:31:34Z

Thanks @spzala .

Actually this PR should be a small and safe change because there is no any change on the JSON output so far, so let me merge it.

ptabor suggested changes Jul 13, 2022

View reviewed changes

Print clusterID, memberID and leaseID in hexdecimal

5cae1a1

ClusterID, memberID and leaseID are just identifies, so it doesn't make sense to print them in decimal. Instead, we should always print them in hexdecimal. Signed-off-by: Benjamin Wang <wachao@vmware.com>

ahrtr force-pushed the print_lease_hex branch from 0971414 to 5cae1a1 Compare July 15, 2022 23:21

spzala reviewed Aug 11, 2022

View reviewed changes

spzala approved these changes Aug 12, 2022

View reviewed changes

ahrtr merged commit 52f367b into etcd-io:main Aug 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Print clusterID, memberID and leaseID in hexdecimal #14208

Print clusterID, memberID and leaseID in hexdecimal #14208

ahrtr commented Jul 11, 2022 •

edited

Loading

ahrtr commented Jul 11, 2022

codecov-commenter commented Jul 11, 2022 •

edited

Loading

neolit123 commented Jul 11, 2022

ptabor left a comment

ptabor Jul 13, 2022

ahrtr Jul 15, 2022

ptabor commented Jul 13, 2022

ahrtr commented Jul 15, 2022 •

edited

Loading

spzala left a comment

ahrtr commented Aug 11, 2022

spzala commented Aug 12, 2022

spzala commented Aug 12, 2022

ahrtr commented Aug 12, 2022

Print clusterID, memberID and leaseID in hexdecimal #14208

Print clusterID, memberID and leaseID in hexdecimal #14208

Conversation

ahrtr commented Jul 11, 2022 • edited Loading

Previous output

New output

ahrtr commented Jul 11, 2022

codecov-commenter commented Jul 11, 2022 • edited Loading

Codecov Report

neolit123 commented Jul 11, 2022

ptabor left a comment

Choose a reason for hiding this comment

ptabor Jul 13, 2022

Choose a reason for hiding this comment

ahrtr Jul 15, 2022

Choose a reason for hiding this comment

ptabor commented Jul 13, 2022

ahrtr commented Jul 15, 2022 • edited Loading

spzala left a comment

Choose a reason for hiding this comment

ahrtr commented Aug 11, 2022

spzala commented Aug 12, 2022

spzala commented Aug 12, 2022

ahrtr commented Aug 12, 2022

ahrtr commented Jul 11, 2022 •

edited

Loading

codecov-commenter commented Jul 11, 2022 •

edited

Loading

ahrtr commented Jul 15, 2022 •

edited

Loading