-
Notifications
You must be signed in to change notification settings - Fork 552
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-2056: net: allow conn_quota logger to be configured #17678
Conversation
Fixes: CORE-2056 Signed-off-by: Noah Watkins <noahwatkins@gmail.com>
Signed-off-by: Noah Watkins <noahwatkins@gmail.com>
Signed-off-by: Noah Watkins <noahwatkins@gmail.com>
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.
lgtm
src/v/net/conn_quota.cc
Outdated
conn_quota::conn_quota(conn_quota::config_fn cfg_f) noexcept | ||
: _cfg(cfg_f()) { | ||
conn_quota::conn_quota( | ||
conn_quota::config_fn cfg_f, seastar::logger* log) noexcept |
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.
nit: here and elsewhere, use ss::
namespace (ss::logger
) instead of seastar::
.
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.
fixed
src/v/net/conn_quota.h
Outdated
@@ -238,6 +236,7 @@ class conn_quota : public ss::peering_sharded_service<conn_quota> { | |||
conn_quota_config _cfg; | |||
|
|||
ss::gate _gate; | |||
seastar::logger* _log; |
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.
nit: If _log
is non-nullable and won't be reseated, prefer a reference over a pointer.
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.
clang-tidy really doesn't like member references. they are a bit weird. wdyt @rockwotj is there a non-null thingy in abseil for this?
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 personally prefer pointers for a few reasons I can elaborate on, but yes, absl has helpers but we have to update absl: https://github.com/abseil/abseil-cpp/blob/master/absl/base/nullability.h
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 personally prefer pointers for a few reasons I can elaborate on
Same, but it's more of a vibe. So I'm curious about your throughts. Maybe we should update the coding-style.md with some reasoning?
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'll open a thread in #cpplang
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.
LGTM
ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/47464#018eafaf-40e5-4275-9681-153fb1eb9057 |
net: allow conn_quota logger to be configured
Backports Required
Release Notes