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

release-21.2: revert "sql: add database ID to sampled query log" #85022

Merged
merged 1 commit into from
Jul 27, 2022

Conversation

THardy98
Copy link

@THardy98 THardy98 commented Jul 25, 2022

Reverts: #84195
This reverts commit c633d13.

Removes the DatabaseID field from the
SampledQuery telemetry log due to the potential of indefinite blocking
in the case of a lease acquisition failure. Protobuf field not reserved as
no official build was released with these changes yet.

Release note (sql change): Removes the DatabaseID field from the
SampledQuery telemetry log due to the potential of indefinite blocking
in the case of a lease acquisition failure.

Release justification: Category 2: Bug fixes and low-risk updates to new functionality

@THardy98 THardy98 requested a review from a team as a code owner July 25, 2022 19:03
@THardy98 THardy98 requested a review from maryliag July 25, 2022 19:04
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@THardy98 THardy98 requested a review from a team July 25, 2022 19:04
Copy link
Contributor

@maryliag maryliag left a comment

Choose a reason for hiding this comment

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

update your PR title to add release-21.2:... and use only lower case, unless is the name of a variable

:lgtm:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained

@THardy98 THardy98 force-pushed the revert_database_id_21.2 branch from b174ee2 to 0432dae Compare July 25, 2022 19:26
@THardy98 THardy98 changed the title Revert "sql: Add database ID to sampled query log" release-21.2: revert "sql: add database ID to sampled query log" Jul 25, 2022
@THardy98
Copy link
Author

THardy98 commented Jul 25, 2022

update your PR title to add release-21.2:... and use only lower case, unless is the name of a variable

:lgtm:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained

Updated.

@THardy98 THardy98 added blathers-backport This is a backport that Blathers created automatically. and removed blathers-backport This is a backport that Blathers created automatically. labels Jul 25, 2022
Copy link
Contributor

@maryliag maryliag left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @maryliag and @THardy98)


pkg/util/log/eventpb/telemetry.proto line 60 at r2 (raw file):

  // Statement fingerprint ID of the query.
  uint64 statement_fingerprint_id = 12 [(gogoproto.customname) = "StatementFingerprintID", (gogoproto.jsontag) = ',omitempty'];

to keep aligned with 22.1 and mater, go back to fingerprint 13 and add 12 as reserved

Reverts: cockroachdb#84195
This reverts commit c633d13.

Removes the DatabaseID field from the
SampledQuery telemetry log due to the potential of indefinite blocking
in the case of a lease acquisition failure. Protobuf field not reserved as
no official build was released with these changes yet.

Release note (sql change): Removes the DatabaseID field from the
SampledQuery telemetry log due to the potential of indefinite blocking
in the case of a lease acquisition failure.
@THardy98 THardy98 force-pushed the revert_database_id_21.2 branch from 0432dae to 1d4d147 Compare July 27, 2022 15:04
Copy link
Author

@THardy98 THardy98 left a comment

Choose a reason for hiding this comment

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

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


pkg/util/log/eventpb/telemetry.proto line 60 at r2 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

to keep aligned with 22.1 and mater, go back to fingerprint 13 and add 12 as reserved

Reserved 12 and reverted back to 13.
Small gen.go change to accomodate reserved fields in the proto.

Copy link
Contributor

@maryliag maryliag left a comment

Choose a reason for hiding this comment

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

small nit, otherwise :lgtm:

Reviewed 3 of 6 files at r1, 1 of 1 files at r2, 2 of 3 files at r3, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @maryliag and @THardy98)


pkg/util/log/eventpb/telemetry.proto line 59 at r3 (raw file):

  string transaction_id = 11 [(gogoproto.customname) = "TransactionID", (gogoproto.jsontag) = ',omitempty', (gogoproto.moretags) = "redact:\"nonsensitive\""];

  reserved 12;

we usually put the list of reserved at the end, so people can just add items there

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