c/backend: shard_table to be able to notify subscribers #19623
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
/dt |
bashtanov
force-pushed
the
shard_table_notifications
branch
from
843b0b6 to
fd67174
Compare
|
/dt |
|
test failure due to #14898 |
bharathv
reviewed
Jun 18, 2024
bashtanov
force-pushed
the
shard_table_notifications
branch
from
fd67174 to
c943a09
Compare
bharathv
approved these changes
Jun 18, 2024
dotnwat
reviewed
Jun 25, 2024
| raft::group_id g, | ||
| std::optional<ss::shard_id> shard)>; | ||
|
|
||
| notification_id_type register_notification(change_cb_t&& cb) { |
There was a problem hiding this comment.
no need to take r-value reference here
There was a problem hiding this comment.
I can name a couple of reasons to use r-value reference here:
- Hinting move semantics to the caller without making them look into the type definition. Might seem useless here, as how do they know the callback signature, but what if they already have the callback value and are making an attempt to reuse it they will probably realise it won't work a bit earlier.
- Optimizing out actual moves https://godbolt.org/z/qbcManb57. They are probably cheap but not zero-cost.
Any reasons to pass by value?
There was a problem hiding this comment.
the primary reason is that (1) it's already non-copyable so you have to move it--no benefit in semantics and (2) it's not in a hot path, right? so copying 40 bytes during registration is nothing. anyway, nothing wrong with using && but it's nice to default to value semantics where possible. no need to change or anything.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Make shard_table able to notify subscribers. Needed for cross-cluster migrations: as partitions move across shards, shard 0 on each node will re-dispatch partition operations to new shards.
Backports Required
Release Notes