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

sql-telemetry: query_sampling.max_event_frequency public #111594

Merged
merged 1 commit into from Oct 3, 2023

Conversation

emilaleksanteri
Copy link
Contributor

@emilaleksanteri emilaleksanteri commented Oct 2, 2023

Epic: none
Fixes: #108385

Release note (sql change): make max_event_frequency public for public documentation

@cockroach-teamcity
Copy link
Member

cockroach-teamcity commented Oct 2, 2023

CLA assistant check
All committers have signed the CLA.

@blathers-crl
Copy link

blathers-crl bot commented Oct 2, 2023

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR.

Before a member of our team reviews your PR, I have some potential action items for you:

  • Please ensure your git commit message contains a release note.
  • When CI has completed, please ensure no errors have appeared.

I have added a few people who may be able to assist in reviewing:

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@blathers-crl blathers-crl bot added O-community Originated from the community X-blathers-triaged blathers was able to find an owner labels Oct 2, 2023
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link

@florence-crl florence-crl left a comment

Choose a reason for hiding this comment

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

@maryliag please review or ask someone else to review to make sure the code change is correct. thanks!

@maryliag maryliag requested a review from a team October 2, 2023 17:53
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.

Hi @emilaleksanteri , thank you for your contribution!

You're missing an update on a generate file, to have the needed doc file updated you need to run ./dev gen, which will generate something similar to this other PR: https://github.com/cockroachdb/cockroach/pull/106530/files

Reviewed all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @emilaleksanteri)


-- commits line 2 at r1:
We do have a format to commit messages, you need to add Fixes #108385 to this commit message and also a release note, similar to #106530

@emilaleksanteri
Copy link
Contributor Author

Hi @emilaleksanteri , thank you for your contribution!

You're missing an update on a generate file, to have the needed doc file updated you need to run ./dev gen, which will generate something similar to this other PR: https://github.com/cockroachdb/cockroach/pull/106530/files

Reviewed all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @emilaleksanteri)

-- commits line 2 at r1: We do have a format to commit messages, you need to add Fixes #108385 to this commit message and also a release note, similar to #106530

Oh right, sounds good, thank you!

@blathers-crl
Copy link

blathers-crl bot commented Oct 2, 2023

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

Thank you for updating your pull request.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@emilaleksanteri
Copy link
Contributor Author

emilaleksanteri commented Oct 2, 2023

Hi @emilaleksanteri , thank you for your contribution!

You're missing an update on a generate file, to have the needed doc file updated you need to run ./dev gen, which will generate something similar to this other PR: https://github.com/cockroachdb/cockroach/pull/106530/files

Reviewed all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @emilaleksanteri)

-- commits line 2 at r1: We do have a format to commit messages, you need to add Fixes #108385 to this commit message and also a release note, similar to #106530

Hi @emilaleksanteri , thank you for your contribution!

You're missing an update on a generate file, to have the needed doc file updated you need to run ./dev gen, which will generate something similar to this other PR: https://github.com/cockroachdb/cockroach/pull/106530/files

Reviewed all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @emilaleksanteri)

-- commits line 2 at r1: We do have a format to commit messages, you need to add Fixes #108385 to this commit message and also a release note, similar to #106530

Hi, it looks like there is something wrong with my ./dev gen builder on linux_amd64

GoCompilePkg pkg/ccl/gssapiccl/gssapiccl.a [for tool] failed: (Exit 1): builder failed: error executing command (from target //pkg/ccl/gssapiccl:gssapiccl) bazel-out/k8-opt-exec-2B5CBBC6/bin/external/go_sdk/builder_reset/builder compilepkg -sdk external/go_sdk -installsuffix linux_amd64 -tags bazel,gss,bazel,gss -src pkg/ccl/gssapiccl/get_user.go -src ... (remaining 53 arguments skipped)

Do you know if I need to do something with my bazel/bazelisk to make the gen work?

@rickystewart
Copy link
Collaborator

@emilaleksanteri

You should be able to reproduce the issue with bazel build pkg/gen:code. You get a better error message by adding the flag --verbose_failures.

From the beginning of the error message, I'm assuming it's going to be a resolv_wrapper link error. Consider using --config cross instead of --config dev (assuming you have the latter in your .bazelrc.user).

If you don't want to do that, installing the libresolv-wrapper package is an option. (If you are on Ubuntu 22.04, this is not an option.)

Removing -lresolv_wrapper from pkg/ccl/gssapiccl/BUILD.bazel is another option.

@emilaleksanteri
Copy link
Contributor Author

@rickystewart woohoo it worked, thank you very much! It was the .bazelrc.user

@blathers-crl
Copy link

blathers-crl bot commented Oct 2, 2023

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

  • We notice you have more than one commit in your PR. We try break logical changes into separate commits, but commits such as "fix typo" or "address review commits" should be squashed into one commit and pushed with --force
  • When CI has completed, please ensure no errors have appeared.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

Epic: none
Fixes: cockroachdb#108385

Release note (sql change): make max_event_frequency public for public documentation
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.

Awesome! :lgtm:

I'll go ahead and merge it!

bors r+

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

@emilaleksanteri
Copy link
Contributor Author

@maryliag woohoo sounds good!!

@craig
Copy link
Contributor

craig bot commented Oct 3, 2023

Build succeeded:

@craig craig bot merged commit 1268ef1 into cockroachdb:master Oct 3, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-community Originated from the community X-blathers-triaged blathers was able to find an owner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set public = t for sql.telemetry.query_sampling.max_event_frequency cluster setting
5 participants