-
Notifications
You must be signed in to change notification settings - Fork 592
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
[CORE-2144] schema_registry/protobuf: Disable protobuf logging #22633
[CORE-2144] schema_registry/protobuf: Disable protobuf logging #22633
Conversation
This is a regression introduced in redpanda-data@e7fab4a Signed-off-by: Ben Pope <ben@redpanda.com>
Fixes CORE-2144 Signed-off-by: Ben Pope <ben@redpanda.com>
/* | ||
* Disable the logger for protobuf; some interfaces don't allow a pluggable | ||
* error collector. | ||
*/ | ||
google::protobuf::SetLogHandler(nullptr); | ||
|
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.
I guess this is fine... is there any information we'll lose by setting this to nullptr
?
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.
I guess this is fine... is there any information we'll lose by setting this to
nullptr
?
Ideally the interface would take an error collector, such as:
- https://github.com/BenPope/redpanda/blob/3aacc02536a970997c8e707e20db1d7a28ed9a73/src/v/pandaproxy/schema_registry/protobuf.cc#L132
- https://github.com/BenPope/redpanda/blob/3aacc02536a970997c8e707e20db1d7a28ed9a73/src/v/pandaproxy/schema_registry/protobuf.cc#L160
But in this case, not. The only interface that I'm using that doesn't have a collector is the one that produces the this spurious log, which is nearly always wrong, since if it can't decode as plain text, it attempts to decode as proto encoded proto, which is highly likely to succeed. In any case, we are not interested in the information in the log.
I added @jcipar as they also have been using protobuf.
/backport v24.2.x |
/backport v24.1.x |
/backport v23.3.x |
Fixes #17682
Notes to reviewer
There are a few (not necessarily orthogonal) options here:
I went with the simplest. Some of the options are not thread-safe.
Backports Required
Release Notes
Improvements
No syntax specified for the proto file