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

cli: improve send-kv-batch help text and examples #73602

Merged
merged 1 commit into from
Dec 10, 2021

Conversation

erikgrinaker
Copy link
Contributor

Release note: None

@erikgrinaker erikgrinaker requested review from tbg and a team December 8, 2021 10:26
@erikgrinaker erikgrinaker requested a review from a team as a code owner December 8, 2021 10:26
@erikgrinaker erikgrinaker self-assigned this Dec 8, 2021
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@erikgrinaker erikgrinaker force-pushed the send-kv-batch-example branch from fcf4f7b to 5a893cf Compare December 8, 2021 10:36
@erikgrinaker erikgrinaker force-pushed the send-kv-batch-example branch from 5a893cf to f33ce92 Compare December 8, 2021 23:46
Copy link
Contributor

@aliher1911 aliher1911 left a comment

Choose a reason for hiding this comment

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

💯

Help is really useful, will save a bit of time when there's an urgent need!

Copy link
Member

@tbg tbg left a comment

Choose a reason for hiding this comment

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

:lgtm:

I have a follow-up PR on top of this commit, I'll send it when this is in.

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @erikgrinaker)

@erikgrinaker
Copy link
Contributor Author

TFTRs!

bors r=aliher1911,tbg

@craig
Copy link
Contributor

craig bot commented Dec 10, 2021

Build succeeded:

@craig craig bot merged commit 61c06f4 into cockroachdb:master Dec 10, 2021
tbg added a commit to tbg/cockroach that referenced this pull request Dec 10, 2021
CRDB 21.2 already populates `Errors.EncodedError`, so we can remove the
deprecated fields.

I was motivated by this when working on a follow-up to cockroachdb#73602 and
ended up trying to marshal an `Error` to JSON, which panics since
it ends up trying to use `reflect` to get an unexported value (due
to the `customname` tags on the deprecated fields). This behavior
persists even with the `json: "-"` tag, by the way, indicating
that we should never use `(gogoproto.customname)` to make a field
private. It's also just one more papercut that comes with using
`gogoproto` but that's a much bigger fish to fry.

Release note: None
tbg added a commit to tbg/cockroach that referenced this pull request Dec 10, 2021
CRDB 21.2 already populates `Errors.EncodedError`, so we can remove the
deprecated fields.

I was motivated by this when working on a follow-up to cockroachdb#73602 and
ended up trying to marshal an `Error` to JSON, which panics since
it ends up trying to use `reflect` to get an unexported value (due
to the `customname` tags on the deprecated fields). This behavior
persists even with the `json: "-"` tag, by the way, indicating
that we should never use `(gogoproto.customname)` to make a field
private. It's also just one more papercut that comes with using
`gogoproto` but that's a much bigger fish to fry.

Release note: None
@erikgrinaker erikgrinaker deleted the send-kv-batch-example branch December 10, 2021 20:10
tbg added a commit to tbg/cockroach that referenced this pull request Dec 10, 2021
CRDB 21.2 already populates `Errors.EncodedError`, so we can remove the
deprecated fields.

I was motivated by this when working on a follow-up to cockroachdb#73602 and
ended up trying to marshal an `Error` to JSON, which panics since
it ends up trying to use `reflect` to get an unexported value (due
to the `customname` tags on the deprecated fields). This behavior
persists even with the `json: "-"` tag, by the way, indicating
that we should never use `(gogoproto.customname)` to make a field
private. It's also just one more papercut that comes with using
`gogoproto` but that's a much bigger fish to fry.

Release note: None
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.

4 participants