Skip to content

cli: don't call (raftpb.Entry).String#63298

Closed
tbg wants to merge 1 commit intocockroachdb:masterfrom
tbg:stringify-raft-ent
Closed

cli: don't call (raftpb.Entry).String#63298
tbg wants to merge 1 commit intocockroachdb:masterfrom
tbg:stringify-raft-ent

Conversation

@tbg
Copy link
Copy Markdown
Member

@tbg tbg commented Apr 8, 2021

Likely due to golang/gogo proto incompatibilities around our use of the
(gogoproto.nullable) option, calling .String() on a raftpb.Entry
panics.

However, upstream at our currently vendored commit
578ffd578362a8962194c4c0c68c1a9824cf9ea3,

func TestFoo(t *testing.T) {
        ent := pb.Entry{Term: 1, Index: 2}
        _ = ent.String()
}

does not panic.

This commit adds a test documenting the undesired behavior and working
around it in ./cockroach debug raft-log (and the other debug commands
that stringify log entries).

Found while investigating #61990.

Release note: None

Likely due to golang/gogo proto incompatibilities around our use of the
`(gogoproto.nullable)` option, calling `.String()` on a `raftpb.Entry`
panics.

However, upstream at our currently vendored commit
578ffd578362a8962194c4c0c68c1a9824cf9ea3,

```
func TestFoo(t *testing.T) {
        ent := pb.Entry{Term: 1, Index: 2}
        _ = ent.String()
}
```

does not panic.

This commit adds a test documenting the undesired behavior and working
around it in `./cockroach debug raft-log` (and the other debug commands
that stringify log entries).

Found while investigating cockroachdb#61990.

Release note: None
@tbg tbg requested a review from a team April 8, 2021 09:13
@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

Copy link
Copy Markdown
Contributor

@erikgrinaker erikgrinaker left a comment

Choose a reason for hiding this comment

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

This is unfortunate. :/

Reviewed 3 of 3 files at r1.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @tbg)

@tbg tbg closed this Apr 16, 2021
@tbg
Copy link
Copy Markdown
Member Author

tbg commented Apr 16, 2021

Has since been fixed, see #63299

@tbg tbg deleted the stringify-raft-ent branch April 16, 2021 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants