Skip to content
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.

Sign up for GitHub

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

c/backend: shard_table to be able to notify subscribers #19623

Merged
merged 1 commit into from
Jun 19, 2024
Merged

c/backend: shard_table to be able to notify subscribers #19623

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions src/v/cluster/shard_table.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,12 @@

#include "base/seastarx.h"
#include "cluster/logger.h"
#include "cluster/notification.h"
#include "container/chunked_hash_map.h"
#include "model/fundamental.h"
#include "model/ktp.h"
#include "raft/fundamental.h"
#include "utils/notification_list.h"

#include <seastar/core/reactor.hh> // shard_id

Expand Down Expand Up @@ -74,6 +76,8 @@ class shard_table final {
log_rev);
_ntp_idx.insert_or_assign(ntp, shard_revision{shard, log_rev});
_group_idx.insert_or_assign(g, shard_revision{shard, log_rev});

_notification_list.notify(ntp, g, shard);
}

void
Expand Down Expand Up @@ -104,6 +108,21 @@ class shard_table final {
log_rev);
_ntp_idx.erase(ntp);
_group_idx.erase(g);

_notification_list.notify(ntp, g, std::nullopt);
}

using change_cb_t = ss::noncopyable_function<void(
const model::ntp& ntp,
raft::group_id g,
std::optional<ss::shard_id> shard)>;

notification_id_type register_notification(change_cb_t&& cb) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to take r-value reference here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can name a couple of reasons to use r-value reference here:

  1. 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.
  2. Optimizing out actual moves https://godbolt.org/z/qbcManb57. They are probably cheap but not zero-cost.

Any reasons to pass by value?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return _notification_list.register_cb(std::move(cb));
}

void unregister_delta_notification(cluster::notification_id_type id) {
_notification_list.unregister_cb(id);
}

private:
Expand Down Expand Up @@ -131,5 +150,7 @@ class shard_table final {
_ntp_idx;
// raft index
chunked_hash_map<raft::group_id, shard_revision> _group_idx;

notification_list<change_cb_t, notification_id_type> _notification_list;
};
} // namespace cluster