WIP - clean up gogo: POC by liggitt · Pull Request #21261 · etcd-io/etcd

liggitt · 2026-02-06T09:10:21Z

Parts of this split out and merged as:

Other work: remove gogo use in https://github.com/etcd-io/raft

Remaining commit breakdown:

pre-work internal-only cleanup (can be pulled out to individual PRs to pass CI and merge)

cleanup: use nil-safe getters
test: use protocmp.Transform
vet: fix error message construction
internal: pass messages by pointer

pre-work with potential performance implications (needs review for correctness and performance impact, can be pulled out to an individual PR to pass CI and merge)

internal: clone messages instead of shallow copy

pre-work with user-facing Go API implications (POC-only, there may be better ways to accomplish the same goal for each of these, these need review and agreement; can be pulled out to individual PRs to pass CI and merge)

client: stop wrapping mvccpb.Event type
client: stop wrapping authpb.UserAddOptions type
client: stop passing proto by value
client: fix Cmp proto pass by value
server: stop passing Snapshot by value
server: stop passing mvccpb.KeyValue by value
- need to resolve data races from mutating shared events https://github.com/etcd-io/etcd/pull/21261/files#r2819339762
etcdctl: stop passing proto by value

generator changes

Switch to standard protoc-gen-go
Drop gogo reference
Drop grpc-gateway gogo message v2 hack

regenerate / recover:

generate: scripts/genproto.sh
recover: Use proto.Marshal/Unmarshal/Size/Clone, proto v2 interface
recover: shim etcdctl JSON marshaling/unmarshaling of TxnResponse (needs review / agreement, json unmarshal of proto one-of field was a little weird before)
generate: scripts/fix/mod-tidy.sh

/hold POC only, not merge-ready

k8s-ci-robot · 2026-02-06T09:10:26Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: liggitt
Once this PR has been reviewed and has the lgtm label, please assign spzala for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot · 2026-02-06T09:10:32Z

Hi @liggitt. Thanks for your PR.

I'm waiting for a etcd-io member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

serathius · 2026-02-06T09:16:05Z

/ok-to-test

serathius · 2026-02-06T09:29:46Z

Now I'm little scared to ask @liggitt for guidance. Really appreciate the help.

codecov · 2026-02-06T09:35:41Z

Codecov Report

❌ Patch coverage is 67.75510% with 79 lines in your changes missing coverage. Please review.
✅ Project coverage is 53.50%. Comparing base (f0c0ca0) to head (f71d836).
⚠️ Report is 150 commits behind head on main.

⚠️ Current head f71d836 differs from pull request most recent head d149059

Please upload reports for the commit d149059 to get more accurate results.

Files with missing lines	Patch %	Lines
server/etcdserver/v3_server.go	37.93%	18 Missing ⚠️
server/storage/wal/wal.go	82.97%	2 Missing and 6 partials ⚠️
server/etcdserver/api/v3rpc/interceptor.go	44.44%	5 Missing ⚠️
server/etcdserver/apply/apply_v2.go	70.58%	3 Missing and 2 partials ⚠️
server/storage/storage.go	50.00%	5 Missing ⚠️
etcdutl/etcdutl/bucket_command.go	0.00%	4 Missing ⚠️
pkg/pbutil/pbutil.go	66.66%	2 Missing and 2 partials ⚠️
server/lease/leasehttp/http.go	55.55%	0 Missing and 4 partials ⚠️
client/v3/concurrency/election.go	0.00%	2 Missing ⚠️
etcdutl/etcdutl/common.go	33.33%	2 Missing ⚠️
... and 18 more

Additional details and impacted files

Files with missing lines	Coverage Δ
client/v3/mock/mockserver/mockserver.go	`0.00% <ø> (ø)`
etcdutl/snapshot/v3_snapshot.go	`58.58% <100.00%> (+0.28%)`	⬆️
server/etcdserver/api/v3election/election.go	`2.77% <100.00%> (-59.30%)`	⬇️
server/etcdserver/api/v3lock/lock.go	`10.00% <100.00%> (-66.48%)`	⬇️
server/etcdserver/api/v3rpc/auth.go	`18.47% <ø> (-33.01%)`	⬇️
server/etcdserver/api/v3rpc/key.go	`13.69% <ø> (-66.04%)`	⬇️
server/etcdserver/api/v3rpc/lease.go	`3.89% <ø> (-77.93%)`	⬇️
server/etcdserver/api/v3rpc/maintenance.go	`35.05% <ø> (-38.72%)`	⬇️
server/etcdserver/api/v3rpc/member.go	`56.45% <ø> (-37.10%)`	⬇️
server/etcdserver/apply/apply.go	`47.22% <100.00%> (-45.64%)`	⬇️
... and 47 more

... and 124 files with indirect coverage changes

@@             Coverage Diff             @@
##             main   #21261       +/-   ##
===========================================
- Coverage   68.37%   53.50%   -14.88%     
===========================================
  Files         429      429               
  Lines       35391    35355       -36     
===========================================
- Hits        24200    18915     -5285     
- Misses       9785    15133     +5348     
+ Partials     1406     1307       -99

Continue to review full report in Codecov by Sentry.

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

serathius · 2026-02-06T10:56:29Z

I think the PR clarifies a lot on the work that needs to be done, however it will take some time to fix tests and push it through review. @liggitt do you plan to finish it?

I think the etcd community could do the heavy lifting if needed. Let me know if you need help with next steps.

ahrtr · 2026-04-08T19:56:55Z

+type TxnResponseJSON struct {
+	Header    *pb.ResponseHeader `json:"header,omitempty"`
+	Succeeded bool               `json:"succeeded"`
+	Responses []ResponseOpJSON   `json:"responses,omitempty"`
+}
+type ResponseOpJSON struct {
+	Response ResponseOpResponseJSON `json:"Response"`
+}
+type ResponseOpResponseJSON struct {
+	ResponseRange       *pb.RangeResponse       `json:"response_range,omitempty"`
+	ResponsePut         *pb.PutResponse         `json:"response_put,omitempty"`
+	ResponseDeleteRange *pb.DeleteRangeResponse `json:"response_delete_range,omitempty"`
+	ResponseTxn         *pb.TxnResponse         `json:"response_txn,omitempty"`
+}


Is the purpose of these change to keep the behaviour of gogo/protobuf? Can we not add these patch?

Users may use etcdctl or use client SDK, the TXN response will not be consistent if we add these patch. We just need to document the behaviour change for etcdctl.

cc @fuweid @liggitt

The goal was to preserve the current user-visible json output of etcdctl for these types. I'm not 100% sure I accomplished that, or whether users have scripted against etcdctl json output (that seems likely)

We just need to document the behaviour change for etcdctl

I wasn't sure changing that wire format was acceptable

The goal was to preserve the current user-visible json output of etcdctl for these types

Yes, I understood that now. But as I mentioned above, the JSON output of TXN response of etcdctl won't be consistent with etcd client SDK anymore. Note that users may use etcd client SDK or etcdctl. I think the former (using sdk) should be more common if they want to programmatically parse the TXN json output.

It depends on how many users are using etcdctl txn -w json in production.
Personally, I am ok with making this break change :)

@fuweid yes, using protojson for all messages is a breaking change, we shouldn't do it.

Let's aim to keep etcdctl's fully compatible with etcd v3.6's behaviour.

Hi @liggitt are you still working on this? I noticed you’ve been pushing commits to keep it up to date. If you’d like to get it ready for review, please let us know. Otherwise, I can split this into smaller changes and send them as a few separate pull requests. (commits are almost ready) Thanks.

I'm not really working on advancing this, was mostly rebasing to try to play with the github feature around stacked PRs (https://github.github.com/gh-stack/) but it looks like that doesn't support personal-fork / upstream-PR yet (github/gh-stack#18)

Otherwise, I can split this into smaller changes and send them as a few separate pull requests. (commits are almost ready) Thanks.

Go ahead, thanks!

Thanks! @liggitt :)

ping @serathius @ahrtr We can work (review) on RangeStream first and then I will send the changes after that.

We can work (review) on RangeStream first and then I will send the changes after that.

Why should we wait if there isn’t a PR ready for review? While RangeStream is important, it’s a relatively simple change. In contrast, gogo/protobuf is more complex and will require significantly more effort.

Signed-off-by: Jordan Liggitt <liggitt@google.com>

This could be done more gradually, one proto API directory at a time, or one module at a time Signed-off-by: Jordan Liggitt <liggitt@google.com>

Signed-off-by: Jordan Liggitt <liggitt@google.com>

k8s-ci-robot · 2026-04-14T15:33:55Z

@liggitt: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-etcd-grpcproxy-e2e-amd64	`d149059`	link	true	`/test pull-etcd-grpcproxy-e2e-amd64`
pull-etcd-grpcproxy-e2e-arm64	`d149059`	link	true	`/test pull-etcd-grpcproxy-e2e-arm64`
pull-etcd-e2e-386	`d149059`	link	true	`/test pull-etcd-e2e-386`
pull-etcd-integration-4-cpu-amd64	`d149059`	link	true	`/test pull-etcd-integration-4-cpu-amd64`
pull-etcd-e2e-arm64	`d149059`	link	true	`/test pull-etcd-e2e-arm64`
pull-etcd-e2e-amd64	`d149059`	link	true	`/test pull-etcd-e2e-amd64`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

k8s-ci-robot · 2026-04-18T21:46:07Z

PR needs rebase.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

liggitt · 2026-05-21T13:36:32Z

closing this since the work got picked up by other maintainers

k8s-ci-robot added do-not-merge/work-in-progress do-not-merge/hold area/clientv3 labels Feb 6, 2026

k8s-ci-robot added area/contrib area/etcdctl area/etcdutl area/robustness-testing area/testing needs-ok-to-test labels Feb 6, 2026

k8s-ci-robot added area/tooling size/XXL labels Feb 6, 2026

liggitt mentioned this pull request Feb 6, 2026

Protobuf: cleanup both golang/protobuf and gogo/protobuf #14533

Closed

liggitt force-pushed the protoc branch from c33603f to 50289a1 Compare February 6, 2026 09:15

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels Feb 6, 2026

liggitt force-pushed the protoc branch from 50289a1 to 9d3d4e6 Compare February 6, 2026 09:19

k8s-ci-robot added the needs-rebase label Feb 6, 2026

liggitt force-pushed the protoc branch from 9d3d4e6 to f71d836 Compare February 6, 2026 09:27

k8s-ci-robot removed the needs-rebase label Feb 6, 2026

liggitt force-pushed the protoc branch from f71d836 to 20719d0 Compare February 6, 2026 09:39

k8s-ci-robot added the needs-rebase label Feb 6, 2026

liggitt force-pushed the protoc branch from 20719d0 to bed88f6 Compare February 6, 2026 09:42

k8s-ci-robot removed the needs-rebase label Feb 6, 2026

liggitt force-pushed the protoc branch from bed88f6 to 97e82a9 Compare February 6, 2026 09:45

liggitt force-pushed the protoc branch from c38a777 to a6f5848 Compare March 26, 2026 20:57

k8s-ci-robot added the needs-rebase label Mar 28, 2026

ahrtr reviewed Apr 8, 2026

View reviewed changes

liggitt added 7 commits April 14, 2026 10:31

cleanup: use nil-safe getters

f0e8080

Signed-off-by: Jordan Liggitt <liggitt@google.com>

test: use protocmp.Transform

381b52a

Signed-off-by: Jordan Liggitt <liggitt@google.com>

vet: fix error message construction

a339f2e

Signed-off-by: Jordan Liggitt <liggitt@google.com>

internal: pass messages by pointer

a856405

Signed-off-by: Jordan Liggitt <liggitt@google.com>

internal: clone messages instead of shallow copy

5e1fbd4

Signed-off-by: Jordan Liggitt <liggitt@google.com>

client: stop wrapping mvccpb.Event type

d3dc0b0

Signed-off-by: Jordan Liggitt <liggitt@google.com>

client: stop wrapping authpb.UserAddOptions type

b0d3717

Signed-off-by: Jordan Liggitt <liggitt@google.com>

liggitt force-pushed the protoc branch from a6f5848 to 6dccab9 Compare April 14, 2026 14:38

k8s-ci-robot removed the needs-rebase label Apr 14, 2026

liggitt added 12 commits April 14, 2026 11:06

client: stop passing proto by value

b630ee0

Signed-off-by: Jordan Liggitt <liggitt@google.com>

client: fix Cmp proto pass by value

b98b4c4

Signed-off-by: Jordan Liggitt <liggitt@google.com>

server: stop passing Snapshot by value

675ac74

Signed-off-by: Jordan Liggitt <liggitt@google.com>

server: stop passing mvccpb.KeyValue by value

f6bf4a5

Signed-off-by: Jordan Liggitt <liggitt@google.com>

etcdctl: stop passing proto by value

51748ed

Signed-off-by: Jordan Liggitt <liggitt@google.com>

Switch to standard protoc-gen-go

205ea5f

This could be done more gradually, one proto API directory at a time, or one module at a time Signed-off-by: Jordan Liggitt <liggitt@google.com>

Drop gogo reference

20b0406

Signed-off-by: Jordan Liggitt <liggitt@google.com>

Drop grpc-gateway gogo message v2 hack

b584736

Signed-off-by: Jordan Liggitt <liggitt@google.com>

generate: scripts/genproto.sh

3c163a2

Signed-off-by: Jordan Liggitt <liggitt@google.com>

recover: Use proto.Marshal/Unmarshal/Size/Clone, proto v2 interface

1a45c22

Signed-off-by: Jordan Liggitt <liggitt@google.com>

recover: shim etcdctl JSON marshaling/unmarshaling of TxnResponse

8d88a54

Signed-off-by: Jordan Liggitt <liggitt@google.com>

generate: scripts/fix/mod-tidy.sh

d149059

Signed-off-by: Jordan Liggitt <liggitt@google.com>

fuweid mentioned this pull request May 1, 2026

client/v3: switch to pointer for clientv3.Cmp #21697

Merged

fuweid mentioned this pull request May 11, 2026

mvcc/watch: switch gogopb kv/event paths to pointer slices #21734

Merged

liggitt mentioned this pull request May 21, 2026

Bump go.etcd.io/raft to v3.7.0-beta.0 #21778

Merged

Uh oh!

Conversation

liggitt commented Feb 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Feb 6, 2026

Uh oh!

k8s-ci-robot commented Feb 6, 2026

Uh oh!

serathius commented Feb 6, 2026

Uh oh!

serathius commented Feb 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Feb 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

serathius commented Feb 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Apr 14, 2026

Uh oh!

k8s-ci-robot commented Apr 18, 2026

Uh oh!

liggitt commented May 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

6 participants

liggitt commented Feb 6, 2026 •

edited

Loading

serathius commented Feb 6, 2026 •

edited

Loading

codecov Bot commented Feb 6, 2026 •

edited

Loading

serathius commented Feb 6, 2026 •

edited

Loading