-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
kv,admission: only log empty admission warning for non-release builds #115705
kv,admission: only log empty admission warning for non-release builds #115705
Conversation
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? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @erikgrinaker and @sumeerbhola)
64d07b2
to
f7d5357
Compare
I was missing the bazel import for |
This error message, while useful for debugging, spams the logs with a stack trace which can be distracting when reading the logs. Since AC defaults to skip when there is an empty header, this is not a concern, unless we see real-world performance impact (which we have not). This patch removes it from release builds while we figure out all the sources for missing headers. Informs cockroachdb#112680 Release note: None
f7d5357
to
943f747
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Build succeeded: |
This error message, while useful for debugging, spams the logs with a stack trace which can be distracting when reading the logs.
Since AC defaults to skip when there is an empty header, this is not a concern, unless we see real-world performance impact (which we have not).
This patch removes it from release builds while we figure out all the sources for missing headers.
Informs #112680
Release note: None