Skip to content
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

loqrecovery: fix json field name in replica info file #77301

Merged
merged 1 commit into from
Mar 8, 2022

Conversation

aliher1911
Copy link
Contributor

@aliher1911 aliher1911 commented Mar 2, 2022

Previously protobuf used camelcase name for repeated field
for descriptor change. This was wrong as it was not parsed
correctly by marshaller.
This fix adds explicit name to proto specification to avoid
default name generation.

Release justification: This change is low risk as it fixes
a bug in new functionality.

Release note: None

Fixes #77282

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@aliher1911 aliher1911 self-assigned this Mar 2, 2022
@aliher1911 aliher1911 force-pushed the loq_recovery_fix_protoname branch from 96593e4 to 6384cd0 Compare March 2, 2022 23:08
@aliher1911 aliher1911 marked this pull request as ready for review March 2, 2022 23:09
@aliher1911 aliher1911 requested review from a team as code owners March 2, 2022 23:09
@aliher1911 aliher1911 requested review from tbg and erikgrinaker March 2, 2022 23:09
@aliher1911 aliher1911 force-pushed the loq_recovery_fix_protoname branch from 6384cd0 to 9641854 Compare March 3, 2022 09:27
@erikgrinaker
Copy link
Contributor

Btw, why wasn't this picked up by any end-to-end tests? Do we lack sufficient coverage?

@aliher1911
Copy link
Contributor Author

It was picked up by the roachtest that is not merged yet. But we didn't have a specific test that manages to create a descriptor update for the TestCluster and recover from it. This seem to me like a "bug" in gogoproto or marshaller where marshalling to json generates data that can't be unmarshalled back. I don't think we should be trying to cover all the underlying libraries beside basics.

@aliher1911 aliher1911 force-pushed the loq_recovery_fix_protoname branch 2 times, most recently from cb32996 to 75b55cb Compare March 3, 2022 12:01
@erikgrinaker
Copy link
Contributor

This seem to me like a "bug" in gogoproto or marshaller where marshalling to json generates data that can't be unmarshalled back. I don't think we should be trying to cover all the underlying libraries beside basics.

Sure, but presumably this caused some functionality to break, which indicates gaps in our end-to-end testing.

@aliher1911 aliher1911 force-pushed the loq_recovery_fix_protoname branch 2 times, most recently from c20ec33 to 92c93aa Compare March 3, 2022 13:02
Previously protobuf used camelcase name for repeated field
for descriptor change. This was wrong as it was not parsed
correctly by marshaller.
This fix adds explicit name to proto specification to avoid
default name generation.

Release justification: This change is low risk as it fixes
a bug in new functionality.

Release note: None
@aliher1911 aliher1911 force-pushed the loq_recovery_fix_protoname branch from 92c93aa to ef9f4fb Compare March 3, 2022 13:28
@aliher1911
Copy link
Contributor Author

bors r=erikgrinaker

@craig
Copy link
Contributor

craig bot commented Mar 3, 2022

Build failed:

@aliher1911
Copy link
Contributor Author

bors r=erikgrinaker

@craig
Copy link
Contributor

craig bot commented Mar 7, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Mar 7, 2022

Build failed (retrying...):

craig bot pushed a commit that referenced this pull request Mar 7, 2022
76611: roachpb: add lock table metadata structures r=AlexTalks a=AlexTalks

This change adds the protobuf structures, as well as the method, needed
to capture the state of a replica's lock table.  This is part of the
work coming out of #75541, and is needed to be able to implement the
`QueryLocks` RPC.

Release note: None

77159: roachtest/tests: move prometheus client interface to separate package r=irfansharif a=ajwerner

This way mockgen does not depend on the `roachtest/tests` package.

Touches #76851.

Release justification: non-production code change

Release note: None

77301: loqrecovery: fix json field name in replica info file r=erikgrinaker a=aliher1911

Previously protobuf used camelcase name for repeated field
for descriptor change. This was wrong as it was not parsed
correctly by marshaller.
This fix adds explicit name to proto specification to avoid
default name generation.

Release justification: This change is low risk as it fixes
a bug in new functionality.

Release note: None

Fixes #77282

Co-authored-by: Alex Sarkesian <[email protected]>
Co-authored-by: Andrew Werner <[email protected]>
Co-authored-by: Oleg Afanasyev <[email protected]>
@craig
Copy link
Contributor

craig bot commented Mar 7, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Mar 8, 2022

Build succeeded:

@craig craig bot merged commit 1b7bcfa into cockroachdb:master Mar 8, 2022
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.

loss of quorum recovery: Generated replica info file has incorrect field values
3 participants