treewide: address newer clang-tidy warnings #17401
Conversation
This fails in a bunch of ways right now, so let's disable it. Ideally it's re-enabled later Signed-off-by: Tyler Rockwood <rockwood@redpanda.com>
rockwotj
force-pushed
the
fix-clang-tidy
branch
from
e112b7f
to
8e1a82d
Compare
|
ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46826#018e7cb9-62e6-4872-8e5e-43c8e074eecc ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46850#018e7dd8-2bc4-49c1-94d7-927efb2c690c ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46850#018e7de8-4bb8-48ae-a8e4-455e6e879de4 ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46893#018e80b8-0516-43ec-84e4-af0f29c59884 |
rockwotj
force-pushed
the
fix-clang-tidy
branch
2 times, most recently
from
e696393
to
88fa2f5
Compare
|
Force pushes: fix inverted bools 🤦 |
Cleanup some code: - emplace_back directly - remove extra add node calls that failed Signed-off-by: Tyler Rockwood <rockwood@redpanda.com>
Newer versions of clang tidy treat this as nodiscard, make sure we're not discarding it. Signed-off-by: Tyler Rockwood <rockwood@redpanda.com>
This shouldn't happen as we shouldn't be recoverying in bootstraping, but this is better than dropping the std::error_code Signed-off-by: Tyler Rockwood <rockwood@redpanda.com>
These cropped up in newer clang versions but what we're doing works and is valid AFAIK. Signed-off-by: Tyler Rockwood <rockwood@redpanda.com>
Signed-off-by: Tyler Rockwood <rockwood@redpanda.com>
rockwotj
force-pushed
the
fix-clang-tidy
branch
from
88fa2f5
to
b57d274
Compare
|
Force push: fix some unit tests because |
|
new failures in https://buildkite.com/redpanda/redpanda/builds/46893#018e80a5-f5ac-412d-bd4d-402ee591c22b: |
|
gentle ping |
| @@ -259,15 +256,15 @@ struct partition_balancer_planner_fixture { | |||
|
|
|||
| workers.allocator.local().register_node( | |||
| create_allocation_node(model::node_id(last_node_idx), 4)); | |||
| new_brokers.push_back(model::broker( | |||
| new_brokers.emplace_back( | |||
There was a problem hiding this comment.
I'm confused about this change. It looks like before this change that we added some entries into new_brokers (above), and then later added some more (this line).
But
- remove extra add node calls that failed
Presumably this test was passing, and this commit is about a clean-up. But this is a logic change?
There was a problem hiding this comment.
Before, we re-added all the existing nodes, which caused an error to be returned (that we ignored). Should I split that out into another change?
There was a problem hiding this comment.
ahh i see, nah it's all good. tests passed before, passes after? haha
There was a problem hiding this comment.
tests passed before, passes after?
Yupp :)
| @@ -203,7 +203,14 @@ bootstrap_backend::apply(bootstrap_cluster_cmd cmd, model::offset offset) { | |||
| [o = offset, | |||
| m = cmd.value.recovery_state->manifest, | |||
| b = cmd.value.recovery_state->bucket](auto& recovery_table) { | |||
| recovery_table.apply(o, m, b, wait_for_nodes::yes); | |||
| auto ec = recovery_table.apply(o, m, b, wait_for_nodes::yes); | |||
There was a problem hiding this comment.
This shouldn't happen
throwing sounds good to me, but it sounds like we could also vassert no failure?
There was a problem hiding this comment.
I'm okay with that I had talked to @andrwng about this
There was a problem hiding this comment.
Right, we don't expect to get here since this is happening at bootstrap, and recoveries aren’t allowed to be started if we haven’t bootstrapped yet.
That said, if we want to be more conservative, we could alternatively handle errc::update_in_progress specifically like we do earlier in the function when we bootstrap users.
There was a problem hiding this comment.
I think I'll leave this - if someone else feels strongly feel free to send a followup patch (I will happily review)
There was a problem hiding this comment.
Right, throwing from an apply method is pretty nasty - if this happens when a snapshot is applied, the exception will be converted to an assert, and if this is a regular command, controller stm fiber will stall, leaving the node in a weird state (but at least operational). Pick your poison.
OTOH this method throws in other places, so I guess the patch is not making matters worse :)
|
/backport v23.3.x |
|
@rockwotj why backport? |
|
To fix to use after move in storage and the bootstrap error being dropped. However there were build issues so I gave up |
Running the public build with clang 17 and clang-tidy picks up some new warnings.
There are way too many to fix the not used rvalue warnings, but hopefully we can chip away at those overtime.
Backports Required
Release Notes